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

Properly hide additional requirements between separate imports #323

Merged
merged 3 commits into from
Feb 14, 2023

Conversation

RedTachyon
Copy link
Member

See #314.

This is semi-WiP, it should be ready to review, but I also want to have another look at it before it's merged.

The general idea here is that a few more explicit import statements might be necessary to use experimental features, but fewer things will be automatically brought into scope when doing import gymnasium as gym

The new barriers are as follows:

  • importing gymnasium doesn't import experimental (since we can potentially do wild things there)
  • importing experimental doesn't import wrappers (since wrappers
  • importing wrappers doesn't import any of the conversion wrappers (which carry extra dependencies)

All individual modules can still be imported manually, so this shouldn't block any functionality.

Remove experimental from default root exports

Remove wrappers from default experimental exports
@RedTachyon RedTachyon marked this pull request as ready for review February 12, 2023 16:17
@floringogianu
Copy link
Contributor

Currently running some experiments with this pull request. It already solved some issues I had in my codebase because of the automatic loading of jax/jumpy.

Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

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

I agree this is a temporal solution but we should work out a better solution for the future, in particular when these become default.

As you wish to review the PR before merging it, I won't merge until @RedTachyon is happy

@RedTachyon
Copy link
Member Author

I did my sanity checks, I'm gonna go ahead and merge.

One note - this is not meant to be a temporary solution, but very permanent and something we need to be paying attention to in the future. As we introduce more optional dependencies (jax in the main repository or whatever else), correctly separating the submodules should be one of the main ways of keeping them under control. The only additional thing I can come up with that would be helpful is some clear message like "To use the JaxToTorch wrapper you must have torch installed" when someone tries to import a submodule without the right dependencies, but that's not so important.

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