8000 feat: Add support for hydra style overrides by hemildesai · Pull Request #80 · NVIDIA/NeMo-RL · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: Add support for hydra style overrides #80

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

Merged
merged 3 commits into from
Apr 1, 2025
Merged

Conversation

hemildesai
Copy link
Collaborator
@hemildesai hemildesai commented Mar 25, 2025

What does this PR do ?

Adds support for Hydra style overrides. See https://hydra.cc/docs/advanced/override_grammar/basic/#basic-override-syntax and https://hydra.cc/docs/advanced/override_grammar/extended/#extended-override-syntax

Issues

Closes #60

Usage

  • You can potentially add a usage example below
python examples/run_grpo_math.py +new_key="sort([1,2,3,4])"

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you run the unit tests and functional tests locally? Visit our Testing Guide for how to run tests
  • Did you add or update any necessary documentation? Visit our Document Development Guide for how to write, build and test the docs.

Additional Information

  • ...

@hemildesai hemildesai changed the title Add support for hydra style overrides feat: Add support for hydra style overrides Mar 25, 2025
@terrykong
Copy link
Collaborator

Are we currently mandating that the config be read-only after overrides are applied? If not, can that be added?

@hemildesai
Copy link
Collaborator Author

Are we currently mandating that the config be read-only after overrides are applied? If not, can that be added?

The config is converted to a dict after overrides are applied. The dict can be modified at any place in the code as of now. If we want to freeze the dict then we will need to convert it to a FrozenDict or similar.

@SahilJain314 SahilJain314 added the Run CICD Set to run CI (unset + set to rerun) label Mar 26, 2025
@SahilJain314
Copy link
Collaborator

Are we currently mandating that the config be read-only after overrides are applied? If not, can that be added?

We currently have places where we modify the configs: https://github.com/NVIDIA/reinforcer/blob/main/examples/run_grpo_math.py#L255

how should we deal with this sort of thing? It's not actually obvious to me if we should hard prevent people from using the configs for global variable passing for minor things like this.

@terrykong
Copy link
Collaborator

how should we deal with this sort of thing? It's not actually obvious to me if we should hard prevent people from using the configs for global variable passing for minor things like this.

Could we do frozen dict as default and just expect the user to manually unfreeze to add? I agree forbidding users from adding things into the config is a little extreme. @hemildesai where did tron end up falling here?

@hemildesai
Copy link
Collaborator Author
hemildesai commented Apr 1, 2025

Yeah I think we can enforce freezing and unfreezing of the config dict if/when needed. However, that would be outside the scope of this PR. Let's move the discussion to #106

@hemildesai where did tron end up falling here?
@terrykong Can you expand on what you mean by this?

terrykong
terrykong previously approved these changes Apr 1, 2025
Signed-off-by: Hemil Desai <hemild@nvidia.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
@hemildesai hemildesai added Run CICD Set to run CI (unset + set to rerun) and removed Run CICD Set to run CI (unset + set to rerun) labels Apr 1, 2025
@terrykong terrykong enabled auto-merge (squash) April 1, 2025 18:16
@hemildesai hemildesai added Run CICD Set to run CI (unset + set to rerun) and removed Run CICD Set to run CI (unset + set to rerun) labels Apr 1, 2025
@terrykong terrykong added Run CICD Set to run CI (unset + set to rerun) and removed 8000 Run CICD Set to run CI (unset + set to rerun) labels Apr 1, 2025
@terrykong terrykong merged commit d18af95 into main Apr 1, 2025
11 checks passed
@terrykong terrykong deleted the hemil/hydra-overrides branch April 1, 2025 19:06
yfw pushed a commit that referenced this pull request Apr 2, 2025
Signed-off-by: Hemil Desai <hemild@nvidia.com>
Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
KiddoZhu pushed a commit that referenced this pull request May 6, 2025
Signed-off-by: Hemil Desai <hemild@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Run CICD Set to run CI (unset + set to rerun)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve CLI to error on unexpected overrides
3 participants
0