Skip to content

Conversation

pcmoritz
Copy link
Contributor

Why are these changes needed?

This fixes the fix #41688 -- that fix was the wrong fix since the validation should never change the runtime_env.

It introduced the problem in https://discuss.ray.io/t/query-application-status-api-triggers-re-deployment/22517

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • 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 added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • 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 :(

@pcmoritz pcmoritz requested a review from edoakes May 20, 2025 22:31
@pcmoritz pcmoritz added the go add ONLY when ready to merge, run all tests label May 20, 2025
Copy link
Collaborator

@edoakes edoakes left a comment

Choose a reason for hiding this comment

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

nonblocking comment

cc @israbbani because this is related to the PR you sent me earlier today

@@ -260,8 +260,8 @@ def runtime_env_contains_remote_uris(cls, v):
if v is None:
return

uris = v.get("py_modules", [])
if "working_dir" in v and v["working_dir"] not in uris:
uris = v.get("py_modules", []).copy()
Copy link
Collaborator

Choose a reason for hiding this comment

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

.copy() is a shallow copy. currently for py_modules this should be fine, but probably safer to do copy.deepcopy() just to be paranoid since there shouldn't be any performance concern here

Copy link
Contributor Author

@pcmoritz pcmoritz May 21, 2025

Choose a reason for hiding this comment

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

Thanks, I made the whole thing nondestructive now, that's probably the clearest solution for a validation function like this and also needs less allocations than a deepcopy would need.

@pcmoritz pcmoritz merged commit bfc769a into master May 21, 2025
5 checks passed
@pcmoritz pcmoritz deleted the fix-serve-runtime-env-validation branch May 21, 2025 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-backlog go add ONLY when ready to merge, run all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants