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] Introduce ability to turn off default logging. #4104

Merged
merged 13 commits into from
Mar 1, 2019
Merged

[tune] Introduce ability to turn off default logging. #4104

merged 13 commits into from
Mar 1, 2019

Conversation

richardliaw
Copy link
Contributor

@richardliaw richardliaw commented Feb 20, 2019 •
edited
Loading

Closes #4078.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/12161/
Test FAILed.

hartikainen
hartikainen reviewed Feb 21, 2019
View reviewed changes
python/ray/tune/logger.py Outdated
@@ -84,9 +85,12 @@ def __init__(self,
config,
logdir,
upload_uri=None,
use_default_loggers=True,
Copy link
Contributor

@hartikainen hartikainen Feb 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Would it make more sense to change the argument to loggers instead of use_default_loggers and then just have a default be loggers=(_JsonLogger, _TFLogger, _CSVLogger)? You could provide a DEFAULT_LOGGERS = (_JsonLogger, _TFLogger, _CSVLogger), and if the user wants to add new loggers on top of the default ones, that can just do

from ray.tune.logger import DEFAULT_LOGGERS

unified_logger = UnifiedLogger(..., loggers=(*DEFAULT_LOGGERS, custom_logger), ...)

Copy link
Contributor

Choose a reason for hiding this comment

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

That way could also get rid of the custom_loggers and just have a single loggers argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I see; yeah let me make this change.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/12254/
Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/12256/
Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/12257/
Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/12288/
Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/12292/
Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/12298/
Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/12299/
Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/12306/
Test FAILed.

@richardliaw richardliaw mentioned this pull request Feb 27, 2019
Closed
@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/12371/
Test FAILed.

@richardliaw richardliaw merged commit c695402 into ray-project:master Mar 1, 2019
@richardliaw
Copy link
Contributor Author

Merged as - lint + relevant travis tests passed; jenkins won't pass but tested locally.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/12398/
Test FAILed.

@robertnishihara robertnishihara deleted the tune_turnoffdefault branch March 6, 2019 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hartikainen hartikainen hartikainen left review comments

@ericl ericl ericl approved these changes

Assignees

@ericl ericl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@richardliaw @AmplabJenkins @ericl @hartikainen