Skip to content

Conversation

@ernestum
Copy link
Contributor

@ernestum ernestum commented Jul 5, 2023

Since gym>=0.21.0 and with gymnasium, there is the possibility to add a package: prefix to the Gym ID to ensure that the given package is imported before creating the environment.

More details here: HumanCompatibleAI/imitation#743 (comment)

This PR ensures that this package prefix is being removed from normalized environment names.
It also adds some missing dependencies.

setup.py Outdated
"cloudpickle>=1.6"
"cloudpickle>=1.6",
"gym~=0.21",
"stable-baselines3~=1.7",
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if it's a good thing that this packages depends on SB3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SB3 is used in push_to_hub.py so it makes sense to depend on it?

I set up a venv for huggingface_sb3, installed it there and then wanted to run the tests. Those tests failed due to missing gym and sb3 dependencies.
Therefore I added them.

Why do you think we should not add the dependency?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, that's true, but I think @simoninithomas wanted to switch to gymnasium (so a simple dependency on SB3 should work).

setup.py Outdated
"numpy",
"cloudpickle>=1.6"
"cloudpickle>=1.6",
"gym~=0.21",
Copy link
Contributor

Choose a reason for hiding this comment

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

this prevents hf hub sb3 from using it with gymnasium/sb3, no? is it needed only for the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think right now huggingface_sb3 does not support gymnasium anyways (see #30).

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey there 👋 I'm finishing the gymnasium integration can we remove this requirement? (gym one)

The PR: https://github.com/huggingface/huggingface_sb3/compare/gymnasium-v2

@simoninithomas
Copy link
Contributor

Hey there 👋 , we currently updating the huggingface_sb3 package for gymnasium. I want to merge also your PR before publishing v2.3.

The PR: https://github.com/huggingface/huggingface_sb3/compare/gymnasium-v2

Can we remove:

  • "gym~=0.21"
  • "stable-baselines3~=1.7"
    In your PR?

@ernestum
Copy link
Contributor Author

ernestum commented Aug 7, 2023

Sure I just removed them.

@simoninithomas
Copy link
Contributor

LGTM thanks 🤗 is there's something else you want to add to this PR or I can merge it?

@ernestum
Copy link
Contributor Author

ernestum commented Aug 8, 2023

I just added some tests for the error case. Now I think it is ready.

@simoninithomas
Copy link
Contributor

Perfect I merge it. For the pip package (v2.3) it will be done next thursday (I'm off from Wed to Wed). In the meantime you can install from gymnasium-v2 branch.

Thanks again for your help 🤗

@simoninithomas simoninithomas merged commit 85e8171 into huggingface:main Aug 8, 2023
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.

3 participants