-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Conversation
…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.
…lter for test_ files in setup.py
… fix windows incompatibility issues.
… command to fetch a docker image that is timing out (I think)
… where it might be failing
…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.
There was a problem hiding this 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") |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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
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
scripts/format.sh
to lint the changes in this PR.