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

Allowing passing kwargs #24692

Merged
merged 2 commits into from
May 18, 2022
Merged

Allowing passing kwargs #24692

merged 2 commits into from
May 18, 2022

Conversation

nmatare
Copy link
Contributor

@nmatare nmatare commented May 11, 2022

Allowing passing kwargs (e.g., temperature) to child distributions.

Least effort modification, but will break for distros that don't accept the same init keywords. Other options include pass-through kwargs or extending with child_kwargs...

Why are these changes needed?

So that you can pass keywords to child distributions.

Related issue number

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Allowing passing kwargs (e.g., `temperature`) to child distributions.

Least effort modification, but will break for distros that don't accept the same init keywords. Other options include pass-through kwargs or extending with `child_kwargs`...
Copy link
Member

@avnishn avnishn left a comment

Choose a reason for hiding this comment

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

I'm fine with this change for the most part -- We're going to be changing the way that models and policies are defined so that its a much more flexible and natural experience -- we'll share more details on those changes soon.

@avnishn
Copy link
Member

avnishn commented May 13, 2022

you'll need to run the linting script that we have from the base ray directory ./ci/lint/format.sh

@sven1977
Copy link
Contributor

Looks good. Thanks for the fix @nmatare . I just cleaned up the LINT errors. Waiting for tests to pass again ...

@nmatare
Copy link
Contributor Author

nmatare commented May 14, 2022

Ah, thanks for the linting hint. Will apply that for next time.

Yeah, it's not really robust to different init keywords for child distros. The kwargs will break the code in that case. But since this is all going to change anyway, I'm looking forward to the new API!

@zhe-thoughts
Copy link
Collaborator

@sven1977 @gjoliver Do you think we should back port to ray-1.x? Thanks

@gjoliver
Copy link
Member

I don't think there is a strong reason to backport.
But I will let @sven1977 confirm or take off the tag.

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.

5 participants