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.

Sign up for GitHub

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

Jump to bottom

[tune] verbosity refactor second attempt #12571

Merged
merged 39 commits into from
Dec 4, 2020
Merged

[tune] verbosity refactor second attempt #12571

merged 39 commits into from
Dec 4, 2020

Conversation

krfricke
Copy link
Contributor

@krfricke krfricke commented Dec 2, 2020

Why are these changes needed?

Follow up to #11767, addressing problems raised offline and in #12132.

Main changes:

  • Added setting print_intermediate_tables to progress reporters, overwriting default behavior
  • Overwriting said behavior in RLLibs train.py (always print intermediate tables)

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

Kai Fricke and others added 30 commits October 30, 2020 11:52
Added verbose_log utility function
5836873
Use new has_verbosity interface in trial detail logging
dba7326
refactor
0dd595c
[tune] logger refactor part 1: move classes and utilities to own files
e9d9aa8
[tune] logger refactor part 2: add syncer callback
dca7a1c
Fix circular dependency
96950e2
Merge branch 'tune-logger-1' into tune-logger-2
e1f7892
Fix tests
f774d14
Remove uneeded pretty print copy
51f2914
Merge branch 'tune-logger-1' into tune-logger-2
841454f
Fix logger flush argument
55a39ae
Continue verbosity
19368bf
Merge branch 'tune-logger-1' into tune-verbosity
2d64581
Merge branch 'tune-logger-2' into tune-verbosity
a5035d5 
# Conflicts:
#	python/ray/tune/trial.py
Add trial progress reporter callback. Implemented suggested changes f…
e0651fa 
…rom design doc
Update docstring
9ef6d20
Apply suggestions from code review
131eb06
Apply suggestion from code review
bb21e8d
Merge branch 'tune-logger-1' into tune-logger-2
1b85853 
# Conflicts:
#	python/ray/tune/logger.py
Merge branch 'tune-logger-2' into tune-verbosity
10cdad8 
# Conflicts:
#	python/ray/tune/utils/callback.py
Merge remote-tracking branch 'upstream/master' into tune-logger-2
18ec504 
# Conflicts:
#	python/ray/tune/callback.py
#	python/ray/tune/logger.py
#	python/ray/tune/tests/test_cluster.py
Merge branch 'tune-logger-2' into tune-verbosity
36a6cc3
Merge branch 'master' into tune-verbosity
98992ae 
# Conflicts:
#	python/ray/tune/logger.py
#	python/ray/tune/syncer.py
#	python/ray/tune/trial.py
#	python/ray/tune/tune.py
#	python/ray/tune/utils/callback.py
Fix merge
2dd9b07
Merge remote-tracking branch 'upstream/master' into tune-verbosity
3da5c5f
Rever raylet.cc changes
daeace2
Fix progress reporter test command
0532f33
@krfricke @richardliaw
Update python/ray/tune/progress_reporter.py
404dbb9 
Co-authored-by: Richard Liaw <rliaw@berkeley.edu>
Apply suggestions from code review
10e2f6d
Applied suggestions from code review
d5b4ae2
@krfricke
Copy link
Contributor Author

krfricke commented Dec 2, 2020

@ericl with these additional changes, running rllib train should show the previous behavior per default. It would be great if you could confirm that this is the case.
Running tune.run(Trainer) should already show the default behavior, as the new default verbosity level is 3 (old 2)

ericl
ericl approved these changes Dec 2, 2020
View reviewed changes
Copy link
Contributor

@ericl ericl left a comment

Choose a reason for hiding this comment

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

Looks good, I tested and both train.py and tune.run(trainer) seem to work as before.

@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Dec 2, 2020
@richardliaw
Copy link
Contributor

Unfortunately tests are failing

@sven1977
Copy link
Contributor

sven1977 commented Dec 3, 2020

RLlib attention net tests will not time out anymore (or at least much less often) once we get the attention net PR merged (in-flight).

@richardliaw
Copy link
Contributor

(tests are failing on test_progress_reporter)

@krfricke
Copy link
Contributor Author

krfricke commented Dec 4, 2020

Right, will get to it today!

@krfricke krfricke added tests-ok The tagger certifies test failures are unrelated and assumes personal liability. and removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Dec 4, 2020
@richardliaw richardliaw merged commit 219c445 into ray-project:master Dec 4, 2020
@krfricke krfricke deleted the tune-verbosity-2 branch September 22, 2023 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardliaw richardliaw richardliaw left review comments

@ericl ericl ericl approved these changes

Assignees

@ericl ericl

@richardliaw richardliaw

@krfricke krfricke

Labels
tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@krfricke @richardliaw @sven1977 @ericl