-
Notifications
You must be signed in to change notification settings - Fork 669
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
Adds example on Hydra integration for running environments #67
Closed
Closed
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
a34ddf3
fixes gitignore for outputs
Mayankm96 959b76f
changes Sequence to List in all configclasses
Mayankm96 eb624c1
adds registration of configclass to hydra
Mayankm96 3393258
adapts zero agent for hydra
Mayankm96 f70290e
runs formatter over the code
Mayankm96 44a50cc
updates sorting of hydra and omegaconf in imports
Mayankm96 e143343
Merge branch 'main' into feature/hydra
Mayankm96 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be done with a pretty simple resolver directly in the config like:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's great! I am not an expert in Hydra so any tips are welcomed!
I was planning to have a version of train.py scripts with hydra for all the workflows too since it is quite convinient. I think to handle the agent configs and env configs together, I would need to do some sort of config composing.
Regarding the suggestion, that makes sense but I was wondering if there's a way to keep Hydra only as a thin layer and not embed hydra resolvers internally.
In general, the practice we are trying to follow is that people should be able to use other config management systems too, i.e. we are not binding to a particular solution. Though I can see how this will have some trade-offs since we won't be able to exploit the tool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I completely agree with preserving the ability to use other config management systems. In my workflow, I tend to write dataclasses which are compatible across different configuration systems (both as schema and as direct config structures).
I try to avoid too much "config logic" within the main code base to also avoid:
1.) being locked in to that particular tool
2.) contributing a higher signal-to-noise ratio where config parsing is definitely noise haha
Let me think a bit on whether there's another way to do something like this without using the OmegaConf resolver. One possible route is through a
__post_init__
call.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely with you on removing this piece of "resolving" on the main script side. Probably the post_init call can be used. Optionally, we don't do either of these and just make it a documentation thing that: for cpu/gpu implementation you need to set those particular arguments in the config.
It will make life easier because, from the simulation side, we do sometimes debug by having different
use_gpu
(PhysX buffers) anduse_gpu_pipeline
(staging buffers) flags. What this means is that using "CPU" for simulation (use_gpu=False) but reading its data on GPU (use_gpu_pipeline=True). Though this is a rather niche thing that many won't need.Do you want to start another Github issue (proposal) to discuss recommendations that you have for using Hydra? Probably easier to keep track of this discussion than having it in the PR itself :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that makes sense! I'll setup a github issue for tracking other hydra-related stuff!
The explicit setting + documentation is probably the right move. However, it can be handy to have "opinionated and sane" default behavior for people to quickly jump in when they don't fully understand all of the configurable options yet, that is resolved behind the scenes e.g.
__post_init__
.