Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add evaluation environment specs to dataset metadata #155

Merged
merged 26 commits into from Oct 29, 2023

Conversation

rodrigodelazcano
Copy link
Member

Description

NOTE: this PR might have conflicts with this one: #137

Some datasets can be targeted for evaluation in an environment with different specifications than the environment used to collect the episodes of the dataset. This is the case of the Maze environments in D4RL (Ant/Point).

This PR adds an additional and optional metadata key to store separate specifications for the evaluation environment, eval_env_spec. Main changes are:

  • To add the environment specifications to a dataset: eval_env is an argument that can be passed during dataset creation (collector env and buffer) as a str dataset id, gym.Env or EnvSpec. eval_env will also be used to compute ref_min_score or ref_max_score when required.
  • Reduce redundant code: the metadata generation and the creation of the dataset path in both create_dataset_from_buffers() and create_dataset_from_collector_env() have been merged into two separate functions: _generate_dataset_path() and _generate_dataset_metadata().
  • Recover evaluation environment: an additional boolean argument has been added to the recover_environment() function in every MinariDataset, eval_env. If eval_env is set to True and the eval_env_spec metadata key is present in the dataset, a gym.Env instance of the evaluation environment will be returned. Otherwise, the environment used for collecting the dataset is returned.
  • Add pytest to check evaluatoin environment recovery and creation of datasets by passing eval_env with the different allowed types.

Dataset updates

In this PR I'm also including the AntMaze datasets and updating PointMaze to include eval_env (these datasets have been already uploaded to GCP). The scripts will be updated to add the evaluation environments in both sets of datasets in the following PR's:

AntMaze - rodrigodelazcano/d4rl-minari-dataset-generation#4
PointMaze - rodrigodelazcano/d4rl-minari-dataset-generation#3

Doc updates

Aside from adding the documentation for AntMaze datasets, this PR expands the documentation of each individual dataset by including in the table all of the specifications of a Gymnasium environment , another table for the evaluation environment specs, and notes to explain how to recover the environments. All of these docs are generated automatically. Example of the docs environment specs and note if no evaluation environment is found for the dataset:

image
image
image

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
  • I have run pytest -v and no errors are present.
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I solved any possible warnings that pytest -v has generated that are related to my code to the best of my knowledge.
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Member

@younik younik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @rodrigodelazcano, thank you also for refactoring the utils file!
I just added few minors comments

Comment on lines 85 to 86
env_spec = EnvSpec.from_json(dataset_spec["env_spec"])
env = gym.make(env_spec.id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we support env_spec = None here? You can do a similar workflow you did for eval_env_spec

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we can. Will it be more helpful to wait for this PR #137 to be merged and then I can try to rebase the functionality in this one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, never mind, this is for the docs only. I'll start working on the env_spec=None case as well

Comment on lines 124 to 213
|ID| `{env_id}`|
| Action Space | `{re.sub(' +', ' ', action_space_table)}` |
|ID| `{env_spec.id}`|
| Observation Space | `{re.sub(' +', ' ', observation_space_table)}` |

| Action Space | `{re.sub(' +', ' ', action_space_table)}` |
| entry_point | `{env_spec.entry_point}` |
| max_episode_steps | `{env_spec.max_episode_steps}` |
| reward_threshold | `{env_spec.reward_threshold}` |
| nondeterministic | `{env_spec.nondeterministic}` |
| order_enforce | `{env_spec.order_enforce}`|
| autoreset | `{env_spec.autoreset}` |
| disable_env_checker | `{env_spec.disable_env_checker}` |
| kwargs | `{env_spec.kwargs}` |
| additional_wrappers | `{env_spec.additional_wrappers}` |
| vector_entry_point | `{env_spec.vector_entry_point}` |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can probably factorize this to a function describe_env, to use for env_spec and eval_env_spec. This can also handle the None case

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea! I'll refactorize this part

minari/dataset/minari_dataset.py Outdated Show resolved Hide resolved
Comment on lines 158 to 159
if eval_env and self._eval_env_spec is not None:
return gym.make(self._eval_env_spec)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add a message to the logger (INFO level?) in case eval_env and _eval_env_spec=None.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeap, agree. I will add this.

tests/utils/test_dataset_creation.py Outdated Show resolved Hide resolved
minari/utils.py Show resolved Hide resolved
@rodrigodelazcano
Copy link
Member Author

I've removed flake8:E272 in gen_dataset_md.py in the pre-commit.

Copy link
Member

@younik younik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; I think there are just two typos, then we can merge

docs/_scripts/gen_dataset_md.py Outdated Show resolved Hide resolved
docs/_scripts/gen_dataset_md.py Outdated Show resolved Hide resolved
@younik younik merged commit 98016ac into Farama-Foundation:main Oct 29, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants