[Dashboard] Start the new dashboard

[fyrestone]

Previous PR was reverted #9860, and I can't reopen that PR. So, here is the new PR.

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/latest/.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failure rates at https://ray-travis-tracker.herokuapp.com/.
• Testing Strategy
  • Unit tests
  • Release tests
  • This PR is not tested (please justify below)

[rkooo567]

This still seems to break almost every Windows tests.

[mfitton]

Yeah, 7 tests passing 39 failing is quite high. I'm going to check recent master builds to verify, but I think there is actually something in the new backend that is breaking these Windows tests.

[mfitton]

@fyrestone I checked another branch in open PR (https://github.com/ray-project/ray/pull/10050/checks?check_run_id=986885497) and it's passing 42 / 46 tests, so it definitely seems like there's something in this build breaking windows functionality. Could you please look into this?

[fyrestone]

@fyrestone I checked another branch in open PR (https://github.com/ray-project/ray/pull/10050/checks?check_run_id=986885497) and it's passing 42 / 46 tests, so it definitely seems like there's something in this build breaking windows functionality. Could you please look into this?

I reviewed the log of windows CI, many tests just timeout or failed. But, I don't know why the tests fail. I don't have a windows development environment. All I can do is guess.

[fyrestone]

@mfitton @rkooo567 The Windows CI looks good, the root cause is ParseWindowsCommandLine parse an empty string returns a vector with size == 1 while ParsePosixCommandLine parse an empty string returns an empty vector. I have fixed it in this PR.

hi, @mehrdadn Why not use boost:: program_options for parsing cmdline? I found two ways:

• boost::program_options::split_unix, please refer to: https://www.boost.org/doc/libs/1_71_0/doc/html/boost/program_options/split_unix.html
• boost::tokenizer + boost::program_options::parse_command_line

boost is cross-platform, safe and well tested. If you insist on making your own wheels, tests should be provided.

[mehrdadn]

@fyrestone because it's not that simple, and boost::program_options isn't general-purpose. (That's why there's split_unix but no split_windows.) I think boost::program_options is meant for known command-line syntaxes; it requires you to specify the command-line syntax beforehand. It doesn't work for more general command-lines that you might pass to other processes, which we need to handle here.

Also I should note parse_command_line requires an array to begin with, but if we had that we wouldn't need ParseCommandLine in the first place.

Note that the current behavior is actually not as weird as it might seem: it's in fact impossible to launch a process with an empty command-line on Windows (at least with normal Win32 APIs like CreateProcess; try it out). In fact if you pass an empty string, it will still get passed an argument. You're probably trying to use this function to parse a portion of a command-line, rather than a full one, which is why you're expecting a zero-length string. That's fine (and I agree this behavior is more useful), but hopefully this puts the current behavior into context.

I'm confused about your demand for tests though. I did in fact include extensive tests for this. Please add a test in util_test.cc for empty strings if you would like this behavior. (And I'm not sure what you mean by wheels; were you thinking of Python?)

[fyrestone]

@fyrestone Also I should note parse_command_line requires an array to begin with, but if we had that we wouldn't need ParseCommandLine in the first place.

If it is not easy to parse cmdline in different platforms, we can use a json string of cmdline (a list of cmd parts), and json is
platform independent. I think it's better not to implement platform-related logic.

[mehrdadn]

I would leave it as is.

[ericl]

We should probably be passing these args base64 encoded to avoid anything like parsing...

…

On Wed, Aug 19, 2020, 11:54 PM fyrestone ***@***.***> wrote: @fyrestone <https://github.com/fyrestone> Also I should note parse_command_line requires an array to begin with, but if we had that we wouldn't need ParseCommandLine in the first place. If it is not easy to parse cmdline in different platforms, we can use a json string of cmdline (a list of cmd parts), and json is platform independent. I think it's better not to implement platform-related logic. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#10131 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAADUSS44LGBDP337OGRRMTSBTCCDANCNFSM4QAFZDBA> .

[fyrestone]

@fyrestone because it's not that simple, and boost::program_options isn't general-purpose. (That's why there's split_unix but no split_windows.) I think boost::program_options is meant for known command-line syntaxes; it requires you to specify the command-line syntax beforehand. It doesn't work for more general command-lines that you might pass to other processes, which we need to handle here.

Also I should note parse_command_line requires an array to begin with, but if we had that we wouldn't need ParseCommandLine in the first place.

Note that the current behavior is actually not as weird as it might seem: it's in fact impossible to launch a process with an empty command-line on Windows (at least with normal Win32 APIs like CreateProcess; try it out). In fact if you pass an empty string, it will still get passed an argument. You're probably trying to use this function to parse a portion of a command-line, rather than a full one, which is why you're expecting a zero-length string. That's fine (and I agree this behavior is more useful), but hopefully this puts the current behavior into context.

I'm confused about your demand for tests though. I did in fact include extensive tests for this. Please add a test in util_test.cc for empty strings if you would like this behavior. (And I'm not sure what you mean by wheels; were you thinking of Python?)

My point is one API should behaves the same on different platforms. While ParseCommandLine is not,

• (Linux / Mac) input an empty string; output an empty vector
• (Windows) input an empty string; output a vector with one element.

[mehrdadn]

@fyrestone: I understand. By "as is" I meant after your change. Your change is fine and it addresses that difference. There's no need to switch to JSON.

[mehrdadn]

Suggested change

	if (s.empty()) {
	return {};
	}

@fyrestone On a second thought, I think you probably want to remove this block. Instead, change the beginning of the loop from

  for (bool stop = false, in_dquotes = false; !stop;) {

to:

  while (i < j && (*i == ' ' || *i == '\t')) {
    ++i;  // Skip leading spaces
  }
  for (bool stop = i >= j, in_dquotes = false; !stop;) {

This is because Windows treats a leading space as 1 empty argument (and hence my implementation did the same). However, considering your goal, I think you want to return 0 arguments in that case as well.

In ray/util/util_test.cc:59, you will need to change the corresponding test case as well:

  ASSERT_EQ(ParseCommandLine(R"( a)", win32), ArgList({R"(a)"}));  // Differs from Windows

[mehrdadn]

@fyrestone I really think you want to make this change before merging, otherwise the semantics will become more confusing. Without this change, an empty string will become an empty array, but a single space will become a 1-element array, which is neither here nor there.

[fyrestone]

Thanks, I think it's better to fix the ParseCommandLine perfectly in another PR. No one is more familiar with this code than you, and you can do the best fix in your PR. This PR is to start the new dashboard.