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

[dashboard] remove symbolic link #46461

Merged
merged 8 commits into from
Jul 10, 2024
Merged

[dashboard] remove symbolic link #46461

merged 8 commits into from
Jul 10, 2024

Conversation

aslonnie
Copy link
Collaborator

@aslonnie aslonnie commented Jul 6, 2024

move into ray package, remove the one from root.

@aslonnie aslonnie requested a review from alanwguo July 6, 2024 14:37
@aslonnie aslonnie added the go add ONLY when ready to merge, run all tests label Jul 6, 2024
@aslonnie aslonnie removed the go add ONLY when ready to merge, run all tests label Jul 6, 2024
@aslonnie
[dashboard] remove root symbolic link
4b9739b 
move into ray package

Signed-off-by: Lonnie Liu <[email protected]>
@jjyao
Copy link
Collaborator

jjyao commented Jul 6, 2024

@pcmoritz may have context why we had symlink in the first place and whether we want to remove it

@aslonnie
Copy link
Collaborator Author

aslonnie commented Jul 6, 2024 •
edited
Loading

past comments from @ericl and people:

  • eric: it was originally to make rllib more clickable.
  • eric: it sounds fine to reverse the direction now; this was something we tried a long time ago.
  • sang: dashboard just follows what rllib did
  • koroush: we can totally swap the symlink direction.
  • sven: fine with me, too, for RLlib.
  • ed: yep ant just copied what rllib did for dashboard

I am fixing dashboard first, and then rllib. rllib is a bit tricky to fix as there are some links in the docs that point to rllib dir.

@aslonnie aslonnie added the go add ONLY when ready to merge, run all tests label Jul 6, 2024
@aslonnie
Copy link
Collaborator Author

aslonnie commented Jul 6, 2024

this will make it easier / simpler to build ray on windows and for conda-forge.

alanwguo
alanwguo reviewed Jul 8, 2024
View reviewed changes
Copy link
Contributor

@alanwguo alanwguo left a comment

Choose a reason for hiding this comment

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

high level, I have no concerns with the change.

Although the PR does have a lot of import ordering changes that seem unrelated making it harder to review.

python/ray/dashboard/modules/actor/actor_head.py Outdated
gcs_service_pb2_grpc,
)
from ray.dashboard.datacenter import DataSource, DataOrganizer
from ray.core.generated import gcs_service_pb2, gcs_service_pb2_grpc
Copy link
Contributor

Choose a reason for hiding this comment

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

are these import changes auto-fixed by a linter or tool?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes.. let me try to split these into a separate PR.

dashboard/ dir was previously not included in the formatter..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#46483 PR applying isort without moving the files.

we can approve that first.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated, now dashboard/ files are pure location moving.

@aslonnie aslonnie requested a review from alanwguo July 8, 2024 19:59
can-anyscale
can-anyscale approved these changes Jul 9, 2024
View reviewed changes
Copy link
Collaborator

@can-anyscale can-anyscale left a comment

Choose a reason for hiding this comment

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

approve for doc changes

@aslonnie
Copy link
Collaborator Author

merging with @alanwguo 's blessings.

@aslonnie aslonnie merged commit 27d3d81 into master Jul 10, 2024
5 checks passed
@aslonnie aslonnie deleted the lonnie-240706-dashsym branch July 10, 2024 00:57
@can-anyscale
Copy link
Collaborator

window wheels seem to fail starting on this commit https://buildkite.com/ray-project/postmerge/builds/5344

@aslonnie
Copy link
Collaborator Author

window wheels seem to fail starting on this commit https://buildkite.com/ray-project/postmerge/builds/5344

hmm... looking at the logs the npm building part already succeeded, then the pip install thirdparty vendored files failed..

hongchaodeng added a commit to hongchaodeng/ray that referenced this pull request Jul 11, 2024
@hongchaodeng
setup-dev.py: remove outdated dashboard link
5db60c3 
After ray-project#46461,
the dashboard symlink has been removed.
hongchaodeng added a commit to hongchaodeng/ray that referenced this pull request Jul 11, 2024
@hongchaodeng
setup-dev.py: remove outdated dashboard link
15b304c 
After ray-project#46461,
the dashboard symlink has been removed.

Signed-off-by: hongchaodeng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alanwguo alanwguo alanwguo approved these changes

@can-anyscale can-anyscale can-anyscale approved these changes

@ericl ericl Awaiting requested review from ericl ericl is a code owner

@richardliaw richardliaw Awaiting requested review from richardliaw richardliaw is a code owner

@rkooo567 rkooo567 Awaiting requested review from rkooo567 rkooo567 is a code owner

@jjyao jjyao Awaiting requested review from jjyao jjyao is a code owner

@edoakes edoakes Awaiting requested review from edoakes edoakes is a code owner

Assignees
No one assigned
Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@aslonnie @jjyao @can-anyscale @alanwguo