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.

Sign up for GitHub

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

Jump to bottom

[core] Fix non default dashboard_agent_listen_port not used when connected to the node #33834

Merged
merged 5 commits into from
Mar 31, 2023
Merged

[core] Fix non default dashboard_agent_listen_port not used when connected to the node #33834

merged 5 commits into from
Mar 31, 2023

Conversation

rickyyx
Copy link
Contributor

@rickyyx rickyyx commented Mar 29, 2023 •
edited
Loading

Why are these changes needed?

If a ray node is started with non-default dashboard-agent-listen-port, and when connecting to the node (e.g. from a driver or a from ray job client) in a worker, ray currently uses the default value for the overwritten port.
This PR fixes this by caching the dashboard-agent-listen-port locally, and reading the cached value when connecting to the node, such that workers connected to the node will see the overwritten port value.

Manual testing with repro script in #33397

ray start --dashboard-agent-listen-port 6945 --head 
ray job submit -- python -c 'import ray;ray.init()'

It has the right port, and logs retrievable.
image

Related issue number

Closes #33397
Closes #33664

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

rickyyx added 2 commits March 29, 2023 00:35
@rickyyx
init
eced1dd 
Signed-off-by: rickyyx <rickyx@anyscale.com>
@rickyyx
commment
b98145a 
Signed-off-by: rickyyx <rickyx@anyscale.com>
@rickyyx rickyyx marked this pull request as ready for review March 29, 2023 00:46
rickyyx
rickyyx commented Mar 29, 2023
View reviewed changes
Copy link
Contributor Author

@rickyyx rickyyx left a comment

Choose a reason for hiding this comment

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

We also don't expose dashboard-agent-listen-port as an option for ray.init but only for ray start.

Wondering if we should make it consistent as well.

@rickyyx
lint
9398af3 
Signed-off-by: rickyyx <rickyx@anyscale.com>
@rkooo567
Copy link
Contributor

We also don't expose dashboard-agent-listen-port as an option for ray.init but only for ray start. Wondering if we should make it consistent as well.

I think this was intentional (since ray.init user normally don't use this), but I guess there's nothing to lose by just supporting it.

rkooo567
rkooo567 approved these changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@rkooo567 rkooo567 left a comment

Choose a reason for hiding this comment

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

LGTM once the test issue is resolved

architkulkarni
architkulkarni reviewed Mar 30, 2023
View reviewed changes
dashboard/modules/job/tests/test_job_agent.py
return True

try:
await async_wait_for_condition_async_predicate(verify, retry_interval_ms=2000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Which part do we really intend to retry here? Does it make sense to do the setup once, and only retry the part that checks if the logs have appeared yet?

architkulkarni
architkulkarni approved these changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@architkulkarni architkulkarni left a comment

Choose a reason for hiding this comment

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

Looks good pending the test issue

@rkooo567
Copy link
Contributor

cc @zhe-thoughts for the approval. This is the issue that broke the prod. Also, cc @alanwguo is this still urgent to merge it by 2.4?

@rickyyx
fix
c512e95 
Signed-off-by: rickyyx <rickyx@anyscale.com>
@rickyyx rickyyx added @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. and removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Mar 31, 2023
@lanbochen-anyscale
Copy link

This seems an easy fix, can we merge this into 2.4 so that we don't have to wait for 2.5? We have potential port conflict issue (https://github.com/anyscale/product/issues/18641) in product even though we haven't seen for the dashboard agent port.

@rickyyx rickyyx added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Mar 31, 2023
zhe-thoughts
zhe-thoughts approved these changes Mar 31, 2023
View reviewed changes
Copy link
Collaborator

@zhe-thoughts zhe-thoughts left a comment

Choose a reason for hiding this comment

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

OK this is important enough so let's merge into master. But let's watch release tests for possible issues. Thanks

@rkooo567 rkooo567 merged commit 70fcb2a into ray-project:master Mar 31, 2023
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
@rickyyx @elliottower
[core] Fix non default dashboard_agent_listen_port not used when conn…
5b0942d 
…ected to the node (ray-project#33834)

If a ray node is started with non-default dashboard-agent-listen-port, and when connecting to the node (e.g. from a driver or a from ray job client) in a worker, ray currently uses the default value for the overwritten port.
This PR fixes this by caching the dashboard-agent-listen-port locally, and reading the cached value when connecting to the node, such that workers connected to the node will see the overwritten port value.

Signed-off-by: elliottower <elliot@elliottower.com>
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request May 4, 2023
@rickyyx @ProjectsByJackHe
[core] Fix non default dashboard_agent_listen_port not used when conn…
01fc285 
…ected to the node (ray-project#33834)

If a ray node is started with non-default dashboard-agent-listen-port, and when connecting to the node (e.g. from a driver or a from ray job client) in a worker, ray currently uses the default value for the overwritten port.
This PR fixes this by caching the dashboard-agent-listen-port locally, and reading the cached value when connecting to the node, such that workers connected to the node will see the overwritten port value.

Signed-off-by: Jack He <jackhe2345@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rkooo567 rkooo567 rkooo567 approved these changes

@zhe-thoughts zhe-thoughts zhe-thoughts approved these changes

@architkulkarni architkulkarni architkulkarni approved these changes

Assignees

@architkulkarni architkulkarni

@rkooo567 rkooo567

Labels
tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Milestone
No milestone
5 participants
@rickyyx @rkooo567 @lanbochen-anyscale @architkulkarni @zhe-thoughts