Code review notes

[RedTachyon]

Writing this without fixes for now, things I'm noticing going through the library:

1. The dataset listed in README doesn't seem to exist. This is bad, as that's the first thing a beginner will likely try when trying to interact with the library. We should either swap it, or create it.
2. Remote datasets are 'door-cloned-v0', 'door-expert-v0', 'door-human-v0', ..., at least reading it initially. One issue here is that the dataset names don't really tell you much unless you already know the context - we should probably mention that they're specifically for mujoco, probably specifically for some specific model - maybe we can list the name of the gymnasium env?
3. Trying to do minari.download_dataset("door-human-v0") results in a pretty ugly/uncaught import error for gymnasium_robotics. Since gymnasium-robotics isn't a necessary dependency, we should have a more elegant way of handling this.
4. (3a) The import issue seems to happen because when downloading the dataset, we're creating the environment to get the obs and action spaces. Ignoring even the dependency issue, this seems very redundant - can't we just save the obs/action spaces in the dataset? They should be easy to serialize.
5. Downloading the same dataset twice repeats the download and overwrites it. We explicitly acknowledge this, but imo there should be an overwrite or force argument to do that behavior, and by default it should just skip downloading it. It might be worthwhile to think if we want to do explicit downloads like this in the first place, but that's another thing.
6. EpisodeData is a namedtuple, which leads to the default representation being an enormous printout of all observations, rewards etc. This is not very useful, so we should probably make it a proper dataclass instead, and override the default representation.

Stopping here for now, @rodrigodelazcano let me know if there's anything I'm missing anything for any of those points (like some additional reasons for doing X thing in a certain way that I'm unaware of). Otherwise we can make this into a to-do list of improvements

[balisujohn]

Checklist for completing/addressing code review.

• [x] 1 https://github.com/Farama-Foundation/Minari/pull/80

• [ ] 2

• [x] 3 https://github.com/Farama-Foundation/Minari/pull/86

• [x] 4 https://github.com/Farama-Foundation/Minari/pull/77 (by default, the spaces are loaded from a serialized representation, then the fallback behavior is to load the module to load the env.)

• [x] 5 https://github.com/Farama-Foundation/Minari/pull/90

• [x] 6 https://github.com/Farama-Foundation/Minari/pull/88

[shreyansjainn]

Hi @balisujohn, a couple of pointers mentioned in this issue I wanted to get feedback on.

• for the 2nd one, like Ariel suggested, I think we can name the datasets starting with the env name instead of just mentioning the shorthand. something like AdroitHandDoor-human-v0 instead of just door-human-v0. here I've not mentioned the version of the env as it can cause some confusion having two versions mentioned in the name.

• for 3rd point, before creating the env while downloading a dataset, we can check if the base library of the environment is installed or not, and if not, we can raise an ImportError saying install so and so library to load so and so env data. let me know if this will be acceptable or not - I've already created a PR for this, currently working on solving some pre-commit errors

• for 5th one, currently while downloading the dataset, we are not returning any object, my thought was that if the local installation is present, we can return the MinariDataset Object after loading it from local directory, and even in case of a fresh download, we can load the dataset and return it as we are anyways loading it in that method to access the combined_datasets value. wanted to ask this from the framework design as it'll change the existing returning behavior of download_dataset

[younik]

6. EpisodeData is a namedtuple, which leads to the default representation being an enormous printout of all observations, rewards etc. This is not very useful, so we should probably make it a proper dataclass instead, and override the default representation.

How would you define a better representation for that? Is it important? I added EspiodeData to encapsulate what was a plain tuple returned by the Dataset; my argument against dataclass is that EpisodeData should be immutable

[RedTachyon]

EpisodeData should be immutable

A dataclass can be frozen.

How would you define a better representation for that? Is it important?

Something more concise with the important characteristics of the dataset. Maybe something about the number of steps, obs/action spaces, or simply just the shapes of all the arrays stored inside.

[im-Kitsch]

Hi @balisujohn, a couple of pointers mentioned in this issue I wanted to get feedback on.

• for the 2nd one, like Ariel suggested, I think we can name the datasets starting with the env name instead of just mentioning the shorthand. something like AdroitHandDoor-human-v0 instead of just door-human-v0. here I've not mentioned the version of the env as it can cause some confusion having two versions mentioned in the name.

Hi, @shreyansjainn

I agree with you that we at least directly use environment name. I don't think d4RL's naming style is a good choice,

But let's consider futher of naming style. Though it may cause confusion if two versions mentioned in the name, but what if we consider data-names as a"leaf" of given environment.

I mean, we could load dataset by using minari.load_dataset(env_name, data_name), for dataset we consider stroage structure like:

- env_name/
| -- data-name-1
| -- data-name-2
- Hopper-v2/
| -- medium-v2
| -- expert-v2
| -- random-v0
| -- random -v2
- Hopper-v4
| -- medium -v2
| -- random -v0

I think for long term thinking, definitely there will be different enviornment version with difeerent data versions, in that case only version would be more messy than using two version. Like "door-human-v2" and AndroitDoor, like "Hopper-v2" to "hopper-medium-v2", it's very inconviniennt. I think definitely most of d4rl datasets will be migrated to minari, and considering future data collection, using two-level naming style is the most intuitive solution and very easy to keep compatity.

[younik]

Closing this issue as I believe everything is solved now.

Thanks for the suggestion @im-Kitsch, we included a hierarchical naming structure in Minari 0.5.0