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

Feature/algo eval #1074

Merged
merged 60 commits into from
Apr 20, 2024
Merged

Feature/algo eval #1074

merged 60 commits into from
Apr 20, 2024

Conversation

maxhuettenrauch
Copy link
Collaborator

@maxhuettenrauch maxhuettenrauch commented Mar 12, 2024

Changes

Dependencies

  • New extra "eval"

Api Extension

  • Experiment and ExperimentConfig now have a name, that can however be overridden when Experiment.run() is called
  • When building an Experiment from an ExperimentConfig, the user has the option to add info about seeds to the name.
  • New method in ExperimentConfig called build_default_seeded_experiments
  • SamplingConfig has an explicit training seed, test_seed is inferred.
  • New evaluation package for repeating the same experiment with multiple seeds and aggregating the results (important extension!). Currently in alpha state.
  • Loggers can now restore the logged data into python by using the new restore_logged_data

Breaking Changes

  • AtariEnvFactory (in examples) now receives explicit train and test seeds
  • EnvFactoryRegistered now requires an explicit test_seed
  • BaseLogger.prepare_dict_for_logging is now abstract

@maxhuettenrauch
Copy link
Collaborator Author

@MischaPanch @bordeauxred please have a look

# Conflicts:
#	examples/mujoco/mujoco_env.py
Copy link
Collaborator

@MischaPanch MischaPanch left a comment

Choose a reason for hiding this comment

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

Preliminary review, we can have a closer look together later,

One thing to do already is to separate saving plots from plotting, and/or give the user the possibility to configure how plots should be saved

examples/mujoco/mujoco_ppo_hl_multi.py Outdated Show resolved Hide resolved
examples/mujoco/mujoco_ppo_hl_multi.py Outdated Show resolved Hide resolved
examples/mujoco/tools.py Outdated Show resolved Hide resolved
examples/mujoco/tools.py Outdated Show resolved Hide resolved
examples/mujoco/tools.py Outdated Show resolved Hide resolved
tianshou/highlevel/experiment.py Outdated Show resolved Hide resolved
tianshou/highlevel/experiment.py Outdated Show resolved Hide resolved
tianshou/highlevel/experiment.py Outdated Show resolved Hide resolved
tianshou/highlevel/experiment.py Outdated Show resolved Hide resolved
examples/mujoco/mujoco_ppo_hl_multi.py Show resolved Hide resolved
@maxhuettenrauch
Copy link
Collaborator Author

I had to use contextlib.suppress in order to make the docs build, but at least tests are passing now. I also linked our fork of rliable in the project dependencies as long as the original does not update the dependency on arch.

@MischaPanch
Copy link
Collaborator

MischaPanch commented Apr 17, 2024

I also linked our fork of rliable in the project dependencies as long as the original does not update the dependency on arch.

Thanks, that works! Seems like rliable is no longer maintained, my PR bumping the arch version got no attention. Let's see for how long we don't need to touch it again. If it creates further problems, might be worth to move the functionality over to tianshou or somewhere else

@MischaPanch
Copy link
Collaborator

@maxhuettenrauch as to the contextlib suppression - I don't think that's the best solution. Instead you could adjust the command in the CI, which is currently poetry install --with dev to poetry install --with dev -E evaluation (or something like that).

Note that you can run github actions locally with this, maybe it makes sense to have a poe command for it

@maxhuettenrauch
Copy link
Collaborator Author

Thanks for the recommendation, unfortunately I haven't managed to get act to work (getting ::error::Not Found but no idea what is actually not found)

@MischaPanch MischaPanch marked this pull request as ready for review April 18, 2024 17:04
@MischaPanch
Copy link
Collaborator

Seems like this is ready, right? Gonna run a few tests tomorrow and then merge it

@MischaPanch
Copy link
Collaborator

I couldn't make parallel execution work with joblib. It also might not make too much sense on a single machine, so we can consider using ray for parallelization instead. For now the limitations are documented, so I'd merge this.

In my last commits I improved the plots a bit (axis labels were cut off), added more docs, fixed some installation problems and did minor enhancements to the interfaces

@MischaPanch MischaPanch enabled auto-merge (squash) April 20, 2024 21:57
@MischaPanch MischaPanch enabled auto-merge (squash) April 20, 2024 23:22
@MischaPanch MischaPanch merged commit ade85ab into thu-ml:master Apr 20, 2024
4 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.

4 participants