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

[docs] print dependencies in Documentation job #36683

Merged
merged 1 commit into from
Jun 22, 2023

Conversation

matthewdeng
Copy link
Contributor

@matthewdeng matthewdeng commented Jun 21, 2023

Why are these changes needed?

Printing out the dependencies before the test runs makes it easier to debug any dependency related issues.

image

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • 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 added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • 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 :(

Signed-off-by: Matthew Deng <matt@anyscale.com>
@aslonnie aslonnie self-requested a review June 22, 2023 00:30
Copy link
Collaborator

@aslonnie aslonnie left a comment

Choose a reason for hiding this comment

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

makes it easier to debug any dependency related issues.

it also makes it harder to find the actual test error.

I do not think it is generally best practice, but I do not really care for this particular broken test, so lgtm.

@aslonnie
Copy link
Collaborator

(and add me as reviewer for review request maybe.)

@matthewdeng
Copy link
Contributor Author

@aslonnie

it also makes it harder to find the actual test error.

What do you mean by this?

@matthewdeng matthewdeng merged commit e50e182 into ray-project:master Jun 22, 2023
@aslonnie
Copy link
Collaborator

What do you mean by this?

it means that it generates a lot of lines that are most of the time not related to the test error when the test fails, so the SNR on the test logs becomes even lower, making the test error harder to find.

dumping just-in-case-useful-but-most-of-the-time-useless info into test logs is generally bad.

what you would want is:

  • make the test closer to deterministic and easier to reproduce.
  • increase the SNR in test logs, and make test error as obvious as possible to notice, to lower the bar for people to find the root cause.
  • add debug info only when debugging
  • if must do, shove debug info into some places for extra logs archives like build artifacts or s3 buckets, rather than streaming them to test logs.

like for this case, I suspect the main reason that you need env info this because the python dependencies are not pinned, and every time in history they might resolve into different versions and hard to reproduce. however, rather than trying to fix the root cause of floating dependencies, what you are doing is printing more info to the test logs, making the lint error, if any, even harder to find now, and making it more likely for people to just ignore the error.

:) hope this makes sense.

@matthewdeng
Copy link
Contributor Author

matthewdeng commented Jun 22, 2023

Yes you are absolutely correct here.

  1. Yes, if tests were deterministic and had pinned dependencies, then we may not need this. Unfortunately, this is not the case today. :(
  2. Fortunately, Buildkite collapses the logs in the UI. (Yes, I know it will still convolute the full log file.)
  3. Until (1) is done, we do need to record this historical information so we can identify if any dependencies have changed.
  4. Makes sense, in the past I've also expected to find this information in the Environment tab (though this might not be the same Environment).

Happy to remove these whenever there is a more robust solution in place!

arvind-chandra pushed a commit to lmco/ray that referenced this pull request Aug 31, 2023
Signed-off-by: Matthew Deng <matt@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants