Skip to content

[jobs] Support ray client format of connection string address for external module#22116

Merged
edoakes merged 2 commits intoray-project:masterfrom
nikitavemuri:module_addr_ray_job
Feb 4, 2022
Merged

[jobs] Support ray client format of connection string address for external module#22116
edoakes merged 2 commits intoray-project:masterfrom
nikitavemuri:module_addr_ray_job

Conversation

@nikitavemuri
Copy link
Copy Markdown
Contributor

@nikitavemuri nikitavemuri commented Feb 4, 2022

Why are these changes needed?

Ray client currently supports connection strings for external modules of the format "other_module://", however ray job commands don't support this format because trailing / is removed. Update so ray job commands also support this format.

Related issue number

Closes #22119

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

@nikitavemuri nikitavemuri marked this pull request as ready for review February 4, 2022 17:26
@nikitavemuri nikitavemuri requested a review from edoakes February 4, 2022 17:27
@edoakes
Copy link
Copy Markdown
Collaborator

edoakes commented Feb 4, 2022

@nikitavemuri just to check, this is only fixing other_module:// with no training info, other_module://blah works fine?

Copy link
Copy Markdown
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.

It would be great if we had a test that actually imported a mocked-out module, it's a bit scary that we don't.

@nikitavemuri
Copy link
Copy Markdown
Contributor Author

Yes, other_module://blah does already work with the ray job commands

@nikitavemuri nikitavemuri added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Feb 4, 2022
@edoakes edoakes merged commit d9dc388 into ray-project:master Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests-ok The tagger certifies test failures are unrelated and assumes personal liability.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] ray job commands don't work with RAY_ADDRESS={module}://

2 participants