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

Update and fix dependencies related to mac install #1044

Merged
merged 13 commits into from
Feb 6, 2024

Conversation

maxhuettenrauch
Copy link
Collaborator

@maxhuettenrauch maxhuettenrauch commented Feb 5, 2024

Addresses part of #1015

Dependencies

  • move jsonargparse and docstring-parser to dependencies to run hl examples without dev
  • create mujoco-py extra for legacy mujoco envs
  • updated atari extra
    • removed atari-py and gym dependencies
    • added ALE-py, autorom, and shimmy
  • created robotics extra for HER-DDPG

Mac specific

Other

@MischaPanch MischaPanch self-requested a review February 6, 2024 13:24
README.md Outdated Show resolved Hide resolved
README.md Outdated
- `robotics` (for gymnasium-robotics environments)
- `vizdoom` (for ViZDoom environments)
- `envpool` (for envpool integration)
- `dev` (for development requirements)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't that a group rather than an extra?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you're referring to 'dev', then you're right. 'robotics' and 'vizdoom' should be extras (in the same fashion as 'atari' and 'mujoco' before). Should I just remove it from readme?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, just dev. Feel free to include it separately in the readme if not done yet, as groups are installed with a different command (not --extras)

@@ -60,13 +60,22 @@ def __init__(self, env, action_repeat=3, reward_scale=5, rm_done=True):
def step(self, action):
rew_sum = 0.0
for _ in range(self.action_repeat):
obs, rew, done, info = self.env.step(action)
step_result = self.env.step(action)
if len(step_result) == 4:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should never happen, we use gymnasium envs, not (older) gym envs. How did you run into this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that if it needs fixing in one example, it will need fixing in all of them :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess it was some older example that noone touched in a while. The check for old vs new api is still present in atari_wrapper.py, that's where I just copied it from. You're right though, the Collector assumes terminated and truncated coming from env.step, I'll just delete this part

examples/mujoco/fetch_her_ddpg.py Show resolved Hide resolved
@@ -96,6 +95,8 @@ def test_ddpg(args=get_args()):

# logger
if args.logger == "wandb":
import wandb
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you do it here, you need to do it everywhere. I thought wandb was not an optional dependency, am I wrong?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I checked, it's the only example where wandb is import. I also thought about removing it since wandb currently is only part of the 'dev' group dependencies

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove pls

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@MischaPanch MischaPanch marked this pull request as ready for review February 6, 2024 15:21
@MischaPanch MischaPanch self-requested a review February 6, 2024 15:21
@MischaPanch
Copy link
Collaborator

Thank you, now only a final testing on windows and ubuntu is needed, and #1015 can be resolved. Merging this when the pipeline passed

@MischaPanch MischaPanch enabled auto-merge (squash) February 6, 2024 15:23
@MischaPanch MischaPanch disabled auto-merge February 6, 2024 16:06
@MischaPanch MischaPanch merged commit 5fe9aea into thu-ml:master Feb 6, 2024
4 of 5 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.

2 participants