Icevision: Pickle records

[lgvaz]

Option to pickle records so we don't have to parse all data everytime

This option should be transparent to the user, we can expose it by a optional argument passed to DataParser.

Always a good discussion is where to store this data. Do it store it relative to the current file? Into /tmp? Or into a .mantisshrimp folder in the home directory?

Storing relative to the current file is always annoying when using version control, we have to explicitly not add it to checkout

Example

COCOParser(data, source, use_cached=True)

[Kshitij09]

Are we planning to implement something like tfrecords here? I mean, storing the entire preprocessed dataset and annotations in a single file or individual files for each image ?

[lgvaz]

I had something simpler in mind, just store the processed data as a pickle file so we don't have to parse it all again (useful for very big datasets), with time we can optimize and start thinking if we can do something like tfrecords for speed improvements.

[Kshitij09]

I have one silly doubt, is it possible to load data batch-wise through pickle files? cause loading the whole dataset in memory isn't really manageable.

[lgvaz]

Keep in mind that the records don't contain images in memory, they're just the parsed annotations, currently, there is no way for reading that in batches.

What is the size of your annotations file?

[Kshitij09]

Oh, now I got it. So the discussion is all about records. Sorry I was not aware of the naming system and tfrecords do store images as well, so I got a bit confused.

I feel it's a great idea to pickle the records and will try to learn more about how it could be done.

[lgvaz]

Hmm... Good point about the confusion with the naming of tfrecords, that tought actually never passed my head 😅

[Kshitij09]

One more thing I'd like to discuss, I usually store the parsed annotations and all the relavent data in a dataframe (csv) and build my PyTorch dataset on top of it.

Are there any performance benefits on storing them as pickle file instead?

I read a while ago that @ zach did some comparison of dataframe vs numpy arrays and numpy was way ahead in that benchmark, so what do you think would be an ideal approach? dataframe or pickle records?

[lgvaz]

Are you tagging the right zach? I know Zach Mueller did some experimentation with that on fastai

Are there any performance benefits on storing them as pickle file instead?

Not when training the model, a pickle file would only help so you do have to re-parse your data every time you run the script.

[Kshitij09]

Are you tagging the right zach? I know Zach Mueller did some experimentation with that on fastai

Yeah I know you know him and thought this would be an unnecessary tag so didn't use the original handle.

Not when training the model

I believe it could help while training as well. Let's say you're training in phases or resuming the training from some checkpoint; you might need to re-parse the annotations and pickling the records might have an advantage here.

[lgvaz]

Yeah I know you know him and thought this would be an unnecessary tag so didn't use the original handle.

But you ended tagging someone else 😅 hahahahhaha

I believe it could help while training as well. Let's say you're training in phases or resuming the training from some checkpoint; you might need to re-parse the annotations and pickling the records might have an advantage here.

That's correct, it can help training in this way

[FraPochetti]

I am gonna go with this one!
@lgvaz can you please assign it to me?
Thanks!

[lgvaz]

Done!

I guess the main question here is where to store the pickle file, what would be the most natural for you? In the same folder the script is being run?

[FraPochetti]

It is a very good question.
I think storing in . would be the easiest, but then, as you said, it'd probably be a pain in version control terms.
Where do you store pre-trained models' weights?
Do you create a new folder?
Maybe we can use that.

[FraPochetti]

Ah no, it is torch handling it right?

[lgvaz]

Pytorch handles that correct, and as we can see they opt to use ~/.cache/torch.

We already create this folder ~/.icevision to store the datasets, so maybe we should use that? With the option of choosing a custom path?

[FraPochetti]

I think it could be a good idea.
At which stage is ~/.icevision created?
Shall we check if it exists and, in case not, create it?

[FraPochetti]

Also, you currently support Linux only, right?
E.g. we don't have to handle Windows/Mac paths.

[FraPochetti]

We already create this folder ~/.icevision to store the datasets

Ah, this is created when invoking datasets from icedata, right?
So, we should assume it is not there, if people fiddle around with icevision.

[FraPochetti]

Ok... sorry for all these questions :)
I was thinking, shall we pass use_cached (and path) to parser.parse, instead of the class constructor?
parser.parse would then look if a pickled version of the dataset exists already, load it and return it, OR run the parsing and pickle results.
If we modify that method, all parsers would inherit the behavior as expected (I hope)

[lgvaz]

Ok... sorry for all these questions :)

It actually makes me happy you're asking all these questions! Don't worry!

I was thinking, shall we pass use_cached (and path) to parser.parse, instead of the class constructor?

Yes!! That is a way better idea actually, we don't want the user to call super().__init__() on the parsers he create

At which stage is ~/.icevision created?

Maybe the current way we're doing this is not optimal, but right now it's created when the library is imported (check icevision.utils.data_dir.py), currently we have this function get_data_dir that returns a Path object.

E.g. we don't have to handle Windows/Mac paths.

Because we're using pathlib.Path all of the OSs are covered (at least I think, no problems have been reported so far)

I was thinking, shall we pass use_cached (and path)

Is there any easy (and intuitive to the user) way to combine both arguments into one? The behaviors are:

• Use cached if available
• Force parsing (don't use cache)
• Specify cache folder

Maybe not? It seems it's necessary to have two arguments to cover all use cases.

look if a pickled version of the dataset exists already, load it and return it, OR run the parsing and pickle results.

Exactly! And if a cached version exist we can use the logger to tell the user that is being used, something like:

logger.log("Loading cached records from {}, specify `use_cached=False` to force parsing the records", self.cache_dir)

Something like that, feel free to correct my bad english hahahahahah

[FraPochetti]

Exactly! And if a cached version exists we can use the logger to tell the user that is being used

Awesome! I agree.

Is there any easy (and intuitive to the user) way to combine both arguments into one?

I don't think so. 2 separate arguments are necessary to fully express users' intentions.
Let me recap:

1. We will implement the functionality within parser.parse
2. We will use 2 arguments, use_cached and cache_path (do you agree with the names of the arguments?)

Here the behavior:

• use_cached=True: (default) we check if a pickled dataset exists in cache_path. If yes, we load it and return. If not, we parse, pickle, and return.
• cache_path=None: (default) cache_path defaults to ~/.icevision

This means that the following might happen:

• use_cached=False AND cache_path=None (OR cache_path=my_path): in this case we DON'T check whether a pickled dataset exists in cached_path. We skip this step and we parse, BUT then we still save to cached_path. If this is the first time the parser is run we simply save, otherwise we overwrite the existing pickle file.
  Does this make sense?

Another thing. We didn't agree on a naming convention for the pickle file.
What filename do we use?
This might be tricky, as how do we infer what dataset is being parsed?
Ideally, we'd want to save pets to a pets.pkl file.

[FraPochetti]

Side note:

right now it's created when the library is imported (check icevision.utils.data_dir.py)

How does this happen? get_data_dir is directly invoked nowhere.

[lgvaz]

Does this make sense?

Yes! Let's go with that 🎉

What filename do we use?

What do you think of using the name of the parser as the name of file? We can use self.__class__.__name__ and then maybe convert from camel case to snake case

How does this happen?

In data_dir.py we have this piece of code is not inside any function and get's exectued when the library is imported:

from icevision.imports import *

root_dir = Path.home() / ".icevision"
root_dir.mkdir(exist_ok=True)

data_dir = root_dir / "data"
data_dir.mkdir(exist_ok=True)

[FraPochetti]

Thanks for the explanations!
I'll get to work now ;)

[FraPochetti]

I think I will have to add pickle to the dependency list.
Is that ok?
Any other library you are already using which has this functionality?

[FraPochetti]

Is that ok?

If yes, where shall I add the import pickle statement?
imports.py?

[FraPochetti]

@lgvaz 😁

[FraPochetti]

Btw, to give you a hint, this is how the new parser.parse() function would look like

def camel_to_snake(name):
    name = re.sub("(.)([A-Z][a-z]+)", r"\1_\2", name)
    return re.sub("([a-z0-9])([A-Z])", r"\1_\2", name).lower()

def parse(
    self,
    data_splitter: DataSplitter = None,
    idmap: IDMap = None,
    autofix: bool = True,
    show_pbar: bool = True,
    use_cached: bool = True,
    cache_path: Union[str, Path] = None,
) -> List[List[BaseRecord]]:
    """Loops through all data points parsing the required fields.

    # Arguments
        data_splitter: How to split the parsed data, defaults to a [0.8, 0.2] random split.
        idmap: Maps from filenames to unique ids, pass an `IDMap()` if you need this information.
        show_pbar: Whether or not to show a progress bar while parsing the data.
        use_cached: Whether or not to load records from an existing pickled file.
        cache_path: Path to save records in pickle format.

    # Returns
        A list of records for each split defined by `data_splitter`.
    """
    cache_path = (
        Path(cache_path)
        if cache_path is not None
        else Path.home() / ".icevision" / "data"
    )
    pkl_data = cache_path / (camel_to_snake(self.__class__.__name__) + ".pkl")

    if pkl_data.exists() and use_cached:
        logger.log(
            "AUTOFIX",
            f"Loading cached records from {cache_path}, specify `use_cached=False` to force parsing the records",
        )
        return pickle.load(open(pkl_data, "rb"))
    else:
        idmap = idmap or IDMap()
        data_splitter = data_splitter or RandomSplitter([0.8, 0.2])
        records = self.parse_dicted(show_pbar=show_pbar, idmap=idmap)

        splits = data_splitter(idmap=idmap)
        all_splits_records = []
        if autofix:
            logger.opt(colors=True).info("<blue><bold>Autofixing records</></>")
        for ids in splits:
            split_records = [records[i] for i in ids if i in records]

            if autofix:
                split_records = autofix_records(split_records)

            all_splits_records.append(split_records)

        pickle.dump(all_splits_records, open(pkl_data, "wb"))

        return all_splits_records

[lgvaz]

Hi! Sorry for the delay 😅

I will have to add pickle to the dependency list.

You can add it to imports.py as you commented afterwards but it's not necessary to add it to setup.cfg

---

Is it a good idea to use another folder that is not data here?

else Path.home() / ".icevision" / "data"

Maybe "records"? Also, you can use get_data_dir for getting that path, and maybe we should add get_root_dir for .icevision?
The idea is that if we decide to change the path in the future we just have to modify it in one place.

Here you should use logger.info instead, "AUTOFIX" should be reserved to the autofix operations under the record only:

logger.log("AUTOFIX", ...)

---

Everything else looks great!!

Just a final consideration, I was thinking again about the use cases, can you think of a scenario where you would not like to save the pickled records? Maybe when the dataset is so small that it takes almost no time to parse? Is it worth adding a way of achieving this?

One way would be to change a bit cache_path, the default can be:

cache_path = Path.home() / ".icevision" / "data"

and when the user passes None we can disable pickling (loading and saving).

[FraPochetti]

Hi! Sorry for the delay

No worries at all man! I must have tons of people pinging you randomly :D

and when the user passes None we can disable pickling (loading and saving).

Agreed!

Here you should use logger.info instead

Got it. Will fit it.

Is it a good idea to use another folder that is not data here?

So basically you are suggesting to add a little def get_root_dir(): return root_dir function to data_dir.py to grab the root directory and create root_dir / "records" inside parser.parse.
Is my understanding correct?

[FraPochetti]

and when the user passes None we can disable pickling (loading and saving).

I am having second thoughts here. I actually believe that use_cached=False should disable both loading and saving.
Implementing the behavior you suggest with cache_path=None is a little confusing.
I think it is more intuitive from a user's perspective to expect such a thing when specifying use_cached=False.
Wdyt?

[FraPochetti]

Let me open a PR so we can discuss on actual code ;)

[FraPochetti]

PR opened here.
As I write there too, I added get_root_dir to data_dir.py but tests are failing because they cannot find the function.
Am I screwing something up with the imports?

FAILED parsers/test_coco_parser.py::test_bbox_parser - NameError: name 'get_root_dir' is not defined
FAILED parsers/test_coco_parser.py::test_mask_parser - NameError: name 'get_root_dir' is not defined
FAILED parsers/test_parser.py::test_parser - NameError: name 'get_root_dir' is not defined
FAILED parsers/test_voc_parsers.py::test_voc_annotation_parser - NameError: name 'get_root_dir' is not defined
FAILED parsers/test_voc_parsers.py::test_voc_mask_parser - NameError: name 'get_root_dir' is not defined

[lgvaz]

Am I screwing something up with the imports?

Heh, you have to add the function to the __all__ list on the top of the file.

use_cached=False should disable both loading and saving.

Hm... This makes sense, basically, if you're loading you don't need to save. Would use_cache=False save still save a pickle file? The big question is, do we need an option that allows the user to decide whether or not to save the pickle file?

I think a common use case would be: Let's say you already parsed your data and the pickle file was created. You changed something on your dataset and now you would like to parse and save it again, what combination of arguments would achieve that?

[FraPochetti]

Thanks for the __all__ tip man!

You are right. Currently we are either lacking an argument to the function (maybe overwrite set to True could fix that?) or not using the 2 arguments we have wisely.

[FraPochetti]

Let me think about this.

[lgvaz]

Let me think about this.

I was just thinking as well hahahah, quite a tricky situation it is

I think this is the updated list of use cases:

• Load from the cache if available
  
  
  
  • If cache was available: nothing else to do, no need to save (because pickle already exists)
  
  
  • If cache was not available, two options: save or not to save pickle
  
  
• Force parsing, two options: save or not to save pickle
• Custom cache folder

Now, what we really have to decide is, is it important to give the option of not saving the pickle file?

[FraPochetti]

Now, what we really have to decide is, is it important to give the option of not saving the pickle file?

Let's not give this option. We save, period.
If the dataset is big, it makes sense to save.
If the dataset is small, saving will make little difference.
So let's simplify and just save.

So, here the combinations:

• use_cached=True + cache_path=None (which defaults to ./icevision/records): if file exists, we load and return. If file does not exist, we parse, save, and return.
• use_cached=False + cache_path=None (which defaults to ./icevision/records): we don't check if the file exists. We parse, save, and return.

To reiterate, we don't give the user the possibility of NOT saving, e.g. we get back to my first implementation.
Wdyt?

[FraPochetti]

I updated the PR according to my thoughts ;)

[FraPochetti]

@lgvaz don't leave me alone! 😭

[lgvaz]

@lgvaz don't leave me alone! 😭

Here I'm!!

Let's not give this option. We save, period.
If the dataset is big, it makes sense to save.
If the dataset is small, saving will make little difference.
So let's simplify and just save.

It was quite funny to read this hahahah, but yes! I totally agree, let's go with that!!

Wdyt?

Perfect! I'll check the PR =)

[FraPochetti]

PR updated