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

[dashboard] Dashboard Test is failing due to tensorboardX 's protobuf dependency issue. #36658

Closed
aslonnie opened this issue Jun 21, 2023 · 8 comments · Fixed by #36643
Closed
Assignees
Labels
bug Something that is supposed to be working; but isn't P0 Issues that should be fixed in short order triage Needs triage (eg: priority, bug/not-bug, and owning component)

Comments

@aslonnie
Copy link
Collaborator

What happened + What you expected to happen

https://buildkite.com/ray-project/oss-ci-build-branch/builds/4563#0188de6c-10fd-4fbf-b52f-999140220081

or

https://buildkite.com/ray-project/oss-ci-build-branch/builds/4568#0188df13-0a86-44d2-81a1-86469747982b

Expects test to pass

Versions / Dependencies

current master

Reproduction script

open a blank PR that touches dashboard with a noop change, and one should be able to see the test failure.

Issue Severity

Medium: It is a significant difficulty but I can work around it.

@aslonnie aslonnie added bug Something that is supposed to be working; but isn't triage Needs triage (eg: priority, bug/not-bug, and owning component) labels Jun 21, 2023
@aslonnie
Copy link
Collaborator Author

https://buildkite.com/ray-project/oss-ci-build-branch/builds/4568#0188df13-0a86-44d2-81a1-86469747982b/3584-3820

The test is pinning tensorboardX at version 2.4.1, which has an old version of protobuf generated python file that is not compatible with the protobuf that the test is using.

@aslonnie
Copy link
Collaborator Author

I tried upgraded tensorboardX to 2.6.1, which uses protobuf>=4, but then will run into protobuf dependency version conflict with other pinned packages.

@aslonnie
Copy link
Collaborator Author

cc @krfricke @can-anyscale @pcmoritz

@matthewdeng
Copy link
Contributor

I took a quick look at some of the Tune jobs (e.g. this) and they are using protobuf 3.19.6. I think some ML dependencies should have this upper bound restriction, not sure why the Dashboard test doesn't though. @krfricke can you take a look and see where the right place to set this is?

@can-anyscale
Copy link
Collaborator

Unrelated to this test failure, but in general, I think it worths an effort to regularly upgrade our library dependencies, and make sure all the versions won't be too stale (e.g. the pinned version is 6 month stale at the maximum). tensorboardX 3.9.x and protobuf 3.19.x all seems to be more than 1 year old, so it's just a matter of time when the are incompatible with other things that get deprecated.

@aslonnie
Copy link
Collaborator Author

and pin everything in tests..

@hora-anyscale
Copy link
Contributor

This was marked as dupe of #36649 which was a P0, so making this P0

@hora-anyscale hora-anyscale added the P0 Issues that should be fixed in short order label Jun 21, 2023
@matthewdeng
Copy link
Contributor

Yep both of these suggestions are in line with @krfricke's work towards unifying dependencies across Docker/CI.

pcmoritz pushed a commit that referenced this issue Jun 22, 2023
which unpins protobuf and uses protobuf>=4

fixes #36658

---------

Signed-off-by: Lonnie Liu <lonnie@anyscale.com>
arvind-chandra pushed a commit to lmco/ray that referenced this issue Aug 31, 2023
which unpins protobuf and uses protobuf>=4

fixes ray-project#36658

---------

Signed-off-by: Lonnie Liu <lonnie@anyscale.com>
Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that is supposed to be working; but isn't P0 Issues that should be fixed in short order triage Needs triage (eg: priority, bug/not-bug, and owning component)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants