search

huggingface / huggingface_sb3

Additional code for Stable-baselines3 to load and upload models from the Hub.
77 stars 23 forks source link

Environment name normalization and explicit naming schemes #13

Closed ernestum closed 2 years ago

ernestum commented 2 years ago

There was an issue with environment names, that have a slash in their name (see https://github.com/DLR-RM/rl-baselines3-zoo/pull/257). Also the naming scheme for models and repository IDs is just based on convention.

This PR implements normalization for environment names (replacing slashes with dashes) and encodes the naming scheme for models and repository IDs in little helper classes. The idea is, that those helper classes can be used by downstream libraries to comply with the naming scheme (such as the rl baselines zoo). If we ever need to change the naming scheme or other cases in which the environment name needs to be normalized come up, then we can implement them here and the downstream libraries immediately profit from that.

I also added a simple smoke test for pulling a model from the hub.

osanseviero commented 2 years ago

I feel this adds some new work under the hood that might be unexpected to users. It also makes loading more complex as users need to use EnvironmentName and ModelName and then use RepoId while as a user I just want to go to the Hub, see ernestumorga/ppo-seals-CartPole-v0 and pass that id directly :thinking:.

araffin commented 2 years ago

I feel this adds some new work under the hood that might be unexpected to users. It also makes loading more complex as users need to use EnvironmentName and ModelName and then use RepoId while as a user I just want to go to the Hub, see ernestumorga/ppo-seals-CartPole-v0 and pass that id directly thinking.

I agree, then I would put that as "advanced usage" because otherwise the current integration will fail with 3rd party env (that was the whole purpose of this PR).

osanseviero commented 2 years ago

SG! Let's maybe not change the whole README based on this then

ernestum commented 2 years ago

I feel this adds some new work under the hood that might be unexpected to users. It also makes loading more complex as users need to use EnvironmentName and ModelName and then use RepoId while as a user I just want to go to the Hub, see ernestumorga/ppo-seals-CartPole-v0 and pass that id directly thinking.

I totally see your point, that this complicates the simple case. My contribution is basically just tools to help you automate uploading and downloading models from the hub (I made this clearer in the docstring I hope).

I agree that the changes to the README should be reverted and we should add a advanced usage section to it. I will do this later/tomorrow unless you have any other proposal.

ernestum commented 2 years ago

Ok ready for second round of review @araffin @osanseviero

ernestum commented 2 years ago

In the codebase we usually go with unittest if possible, here is a clean example https://github.com/huggingface/huggingface_hub/blob/main/tests/test_utils_sha.py, and having assertions through self.assertEqual, etc instead of direct asserts. I think your approach is fine though if it conforms well with sb3 codebase. Thanks again for the PR!

In SB3 we use pytest and I also personally prefer it since it has less boilerplate code. If it is an hard requirement I can of course switch to unittest.

osanseviero commented 2 years ago

The testing is good :smile:

ernestum commented 2 years ago

Awesome. Can we merge this then @osanseviero ?