Moved h5py, google_cloud_storage, pyarrow and tqdm to optional depend… #218
Conversation
…encies. Removed portion and packaging.
There was a problem hiding this comment.
Thank you for the PR @JosephCarrino!
It looks good, I just added a couple of comments. I will also have a deep look for Gymnasium dependency soon.
|
Oh also for Test Documentation, you should change this line to also install all: Minari/.github/workflows/docs-test.yml Line 25 in 338b19a Then, you don't need anymore tqdm in docs/requirements.txt (can you also remove google-cloud-storage==2.5.0 from it? thanks) |
|
|
||
| from minari.data_collector.episode_buffer import EpisodeBuffer | ||
| from minari.dataset.minari_storage import MinariStorage | ||
|
|
||
|
|
||
| class ArrowStorage(MinariStorage): | ||
| FORMAT = "arrow" |
There was a problem hiding this comment.
Tests fails because there are places where FORMAT is used; let's keep FORMAT.
It would be nice to assert in init that the key is the same as MinariStorage.FORMAT. If you have time, change the init to be similar to the of CloudStorage: use a function get_minari_storage(key: str) (that uses the private registry of callables) and assert that storage.FORMAT == key. (Then you will also need to change the places where the registry is used to use the function instead). This is a very minor problem, so it's okay to skip it.
There was a problem hiding this comment.
I should have implemented this too, however this is a bite more tricky than the CloudStorage one, since this registry is used in many tests to get the list of the available storages. What I did is creating also a function get_storage_keys() -> List[str] that preserve the fact of registry being private and returns the list of its keys. I replaced the previous _storages.registry.keys() which was in some tests and functions by this new function.
There was a problem hiding this comment.
Sounds good, can you push the changes? @JosephCarrino
Updated dependency list removing "portion" and "packaging", which are not used.
Some dependencies (i.e., h5py, google-cloud-storage) are now optional and not mandatory anymore, since they are used only for specific features. An
ImportErrorexpection is raised in case one is needed and not installed. Also, tqdm is now indocs/requirements.txt.