Deprecate create_dataset_from_collector_env #169

avjmachine · 2023-11-24T05:28:00Z

Description

Move the function `minari.create_dataset_from_collector_env() to env.create_dataset(). Add deprecation warning for the same.
Add deprecation warning for making env.save_to_disk private.
Both changes planned for release 0.5.0

Fixes # (issue), Depends on # (pull request)

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)
This change requires a documentation update

Screenshots

N/A

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

create_dataset moved to env.create_dataset and env.save_to_disk to be made private

avjmachine · 2023-11-24T06:18:21Z

@younik Faced a circular import issue due to DataCollectorV0 being still used in the create_dataset_from_collector_env argument type, while env.create_data() uses functions from utils.py.
So, I currently changed the argument type to Any to ensure tests run properly. Another option is to move the required functions and their dependencies to data_collector.py and change imports in all other files. Please suggest if there is a better option than both of these.

younik

Very good thanks, I suggested some minor changes

younik · 2023-11-24T17:35:46Z

docs/api/minari_functions.md

@@ -10,7 +10,7 @@ minari_dataset/episode_data
 ## Create Minari Dataset

 ```{eval-rst}
-.. autofunction:: minari.create_dataset_from_collector_env
+.. autofunction:: env.create_dataset


I don't think this generates the documentation properly
Can you check? You can build the doc locally

Couldn't spot it since I had failures in building the docs. Now removed it since it is no longer required due to the deprecation.

younik · 2023-11-24T17:36:11Z

docs/api/data_collector.md

@@ -22,6 +22,7 @@ data_collector_callbacks/episode_metadata_callback
 .. autofunction:: minari.DataCollectorV0.step
 .. autofunction:: minari.DataCollectorV0.reset
 .. autofunction:: minari.DataCollectorV0.close
+.. autofunction:: minari.DataCollectorV0.create_dataset
 .. autofunction:: minari.DataCollectorV0.save_to_disk


Can you remove save_to_disk from doc?

younik · 2023-11-24T17:38:58Z

docs/content/basic_usage.md

@@ -52,7 +52,7 @@ In this example, the :class:`minari.DataCollectorV0` wraps the `'CartPole-v1'` e
 ```{eval-rst}
 To create a Minari dataset first we need to step the environment with a given policy to allow the :class:`minari.DataCollectorV0` to record the data that will comprise the dataset. This is as simple as just looping through the Gymansium MDP API. For our example we will loop through ``100`` episodes of the ``'CartPole-v1'`` environment with a random policy.

-Finally, we need to create the Minari dataset and give it a name id. This is done by calling the :func:`minari.create_dataset_from_collector_env` Minari function which will move the temporary data recorded in the :class:`minari.DataCollectorV0` environment to a permanent location in the `local Minari root path </content/dataset_standards>`_ with the Minari dataset standard structure.
+Finally, we need to create the Minari dataset and give it a name id. This is done by calling the :func:`env.create_dataset` Minari function which will move the temporary data recorded in the :class:`minari.DataCollectorV0` environment to a permanent location in the `local Minari root path </content/dataset_standards>`_ with the Minari dataset standard structure.


In the doc, maybe you should write DataCollectorV0.create_dataset? As env can be unclear what it means

[there are multiple places where this comment is valid]

Modified in all the instances in the docs.

younik · 2023-11-24T17:39:52Z

docs/tutorials/dataset_creation/point_maze_dataset.py

@@ -396,8 +395,7 @@ def __call__(

    obs, rew, terminated, truncated, info = collector_env.step(action)

-dataset = minari.create_dataset_from_collector_env(
-    collector_env=collector_env,
+dataset = env.create_dataset(


to fix tests, env -> collector_env here

younik · 2023-11-24T17:44:15Z

minari/utils.py

@@ -567,7 +566,7 @@ def create_dataset_from_buffers(

 def create_dataset_from_collector_env(
    dataset_id: str,
-    collector_env: DataCollectorV0,
+    collector_env: Any,


to avoid circular import, in DataCollectorV0, you can import from minari.utils import _generate_dataset_metadata, _generate_dataset_path inside the create_dataset function (please also add a #TODO comment saying to move it on top with 0.5.0 release)

This change is also done.

call DataCollectorV0.create_dataset from create_dataset_from_collector_env to avoid duplicate code shift imports for _generate_dataset_metadata and _generate_dataset_path down to create_dataset method to avoid circular imports

younik

LGTM, thanks!

avjmachine added 5 commits November 24, 2023 08:05

add warnings for deprecations

2f37241

create_dataset moved to env.create_dataset and env.save_to_disk to be made private

create env.create_dataset()

5dbd7e2

update docs & tests to use env.create_dataset

eebccff

add create_dataset to list of methods in data_collector.md

274a2ff

ad-hoc fix for circular import issue

41b5d57

younik requested changes Nov 24, 2023

View reviewed changes

avjmachine added 3 commits November 25, 2023 14:27

fix docs for DataCollectorV0.create_dataset

f61915b

fix wrong usage of create_dataset in pointmaze doc

3a52540

fix circular imports, avoid duplicate code

4a43bbf

call DataCollectorV0.create_dataset from create_dataset_from_collector_env to avoid duplicate code shift imports for _generate_dataset_metadata and _generate_dataset_path down to create_dataset method to avoid circular imports

younik approved these changes Nov 25, 2023

View reviewed changes

younik merged commit 320d3fc into Farama-Foundation:main Nov 25, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate create_dataset_from_collector_env #169

Deprecate create_dataset_from_collector_env #169

avjmachine commented Nov 24, 2023 •

edited

avjmachine commented Nov 24, 2023

younik left a comment

younik Nov 24, 2023

avjmachine Nov 25, 2023

younik Nov 24, 2023 •

edited

avjmachine Nov 25, 2023

younik Nov 24, 2023

younik Nov 24, 2023

avjmachine Nov 25, 2023

younik Nov 24, 2023

avjmachine Nov 25, 2023

younik Nov 24, 2023

avjmachine Nov 25, 2023

younik left a comment

Deprecate create_dataset_from_collector_env #169

Deprecate create_dataset_from_collector_env #169

Conversation

avjmachine commented Nov 24, 2023 • edited

Description

Type of change

Screenshots

Checklist:

avjmachine commented Nov 24, 2023

younik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

younik Nov 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

younik left a comment

Choose a reason for hiding this comment

avjmachine commented Nov 24, 2023 •

edited

younik Nov 24, 2023 •

edited