Refactor DataCollectorV0 and HDF5 dependencies isolation

[younik]

Motivation

In the future, we aim to consider different storage methods than HDF5 files for our datasets (see #98), as other backends can be faster.
Currently, different files depend on h5py; this PR isolates the dependency on MinariStorage.
If we add support for other storage, we simply need to create a new MinariStorage that implements the same interface.

Changes

To achieve the h5py isolation, we make DataCollectorV0, functions in utils and MinariDataset to interface with MinariStorage instead of directly to h5py. Thus, MinariStorage, should offer an API for the most used methods.
This makes as natural dependencies DataCollectorV0 -> MinariStorage. and MinariDataset -> MinariStorage.

To avoid MinariDataset -> DataCollectorV0 dependency, we change the current API for adding the buffer of a DataCollector to a MInariDataset, from:

minari_dataset.update_from_env_collector(data_collector)

to:

data_collector.add_to_dataset(minari_dataset)

Other user notable changes:

• MinariStorage doesn't want anymore the path to the file, but the path to the directory containing the file(s).
• The MinariStorage of a MinariDataset is now accessible using .storage
• The init method of MinariStorage is for an existing dataset. To create a new storage, use the class method new
• It is not possible anymore to combine dataset without copying. We may want to add this again in the future (but it will likely be incompatible with different backends).
• Remove the custom error when env module is missing. There is already an informative error from Gymnasium (and if it is not informative enough, it should be changed there).

[balisujohn]

What is the reason for removing these checks?

[younik]

It is not possible anymore to have copy=False, as it was causing problem (the ids of episode were not consistent)

[balisujohn]

I see so this test removal is in combination with removing the copy optional argument? That seems reasonable, assuming we must remove that feature to isolate h5py dependence to MinariStorage

[younik]

It is not specific about h5py isolation, but it is a bug in the current code: when you combine two datasets, the episode ids of the second dataset are modified when you have copy=False, i.e. they don't start anymore from 0

[balisujohn]

I noticed there is still an h5py import in utils.py and in it is used in get_normalized_scores Is this by design, and if so, what is blocking removing the h5py dependency there? Other than this and the other question I had, this seems good overall. It adds new tests and passes the existing tests.

Here are profiling results.

[younik]

I noticed there is still an h5py import in utils.py and in it is used in get_normalized_scores Is this by design?

Oh, thanks for spotting this, it wasn't by design; I fixed it.

[balisujohn]

Just had one more question, otherwise, once this is passing pre-commit, I think it's ready to merge :^)

[balisujohn]

what is the purpose of the kwargs?

[younik]

It is needed for this new test; they are a general kwarg that can be passed to thecreate_dataset_from_collector_env function

Minari/tests/utils/test_get_normalized_score.py

Lines 20 to 27 in c5c88e8

	ref_min_score, ref_max_score = -1, 100
	dataset = create_dummy_dataset_with_collecter_env_helper(
	"cartpole-test-v0",
	env,
	num_episodes=num_episodes,
	ref_min_score=ref_min_score,
	ref_max_score=ref_max_score,
	)

[balisujohn]

Ready to merge :^) but one last question, what it two datasets that are themselves each the result of combining two datasets are combined? Will this situation be handled correctly?

[younik]

Ready to merge :^) but one last question, what it two datasets that are themselves each the result of combining two datasets are combined? Will this situation be handled correctly?

Thanks! Yes, this should not create any issue as the new generated dataset is a perfectly normal dataset (everything is copied); with just different metadata.