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] Turn on new dashboard by default pt 2 #11510

Merged
merged 28 commits into from
Oct 23, 2020

Conversation

mfitton
Copy link
Contributor

@mfitton mfitton commented Oct 20, 2020

Why are these changes needed?

I reverted the previous roll-out due to bad wheel builds + windows errors. This is the next attempt.

Related issue number

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

Max Fitton and others added 24 commits October 19, 2020 18:37
…files are not included in the wheel yet the dashboard modules include test files within the module. This commit makes those test files only import conftest and other testing files if being run as main.
… command to fetch a docker image that is timing out (I think)
…kend needs to be able to start running in order to provide the reporter in the new dash architecture, so we need to make sure this works.
Copy link
Contributor

@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.

LGTM. I only really did a "sanity check" pass but all of the changes look reasonable.

@@ -30,7 +30,7 @@

def setup_static_dir():
build_dir = os.path.join(
os.path.dirname(os.path.abspath(__file__)), "client/build")
os.path.dirname(os.path.abspath(__file__)), "client", "build")
Copy link
Contributor

Choose a reason for hiding this comment

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

is this an intentional change?

@@ -304,7 +302,7 @@ async def _check_parent():
args.redis_address, password=args.redis_password)
traceback_str = ray.utils.format_error_message(traceback.format_exc())
message = ("The agent on node {} failed with the following "
"error:\n{}".format(os.uname()[1], traceback_str))
"error:\n{}".format(platform.uname()[1], traceback_str))
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just use socket.gethostname here instead? I think that's what we use in other places in the code

@edoakes edoakes merged commit caf3b04 into master Oct 23, 2020
@edoakes edoakes deleted the turn-on-new-dashboard-take2 branch October 23, 2020 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants