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

[AIR] Do not deepcopy RunConfig #23499

Merged
merged 1 commit into from
Mar 25, 2022
Merged

Conversation

amogkam
Copy link
Contributor

@amogkam amogkam commented Mar 25, 2022

RunConfig is not a tunable hyperparameter, so we do not need to deep copy it when merging parameters with Ray Tune's param_space.

Why are these changes needed?

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 :(

@Yard1
Copy link
Member

Yard1 commented Mar 25, 2022

I'm wondering why we deepcopy in merge_dicts in the first place. I think it should be just a shallow copy.

@amogkam
Copy link
Contributor Author

amogkam commented Mar 25, 2022

@Yard1 are you saying that instead of doing the deepcopy all up front, we should just copy level by level, as we do the deep merging? If so, I agree- but I think we can leave that for a future PR. The implementation for this just used the merge_dicts utility that we currently have.

@amogkam amogkam merged commit 7fd7efc into ray-project:master Mar 25, 2022
@amogkam amogkam deleted the no-copy-runconfig branch March 25, 2022 20:12
@richardliaw richardliaw added this to the Ray AIR milestone Apr 8, 2022
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.

None yet

5 participants