-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[tune] logger refactor part 3: Add ExperimentLogger class #11749
Conversation
# Conflicts: # python/ray/tune/logger.py
# Conflicts: # python/ray/tune/logger.py # python/ray/tune/utils/callback.py
# Conflicts: # python/ray/tune/callback.py # python/ray/tune/logger.py # python/ray/tune/tests/test_cluster.py
(needs a quick rebase?) |
…er-3 # 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
# Todo(krfricke): Deprecate `loggers` argument, print warning here. | ||
add_loggers = [] | ||
for trial_logger in loggers: | ||
if isinstance(trial_logger, ExperimentLogger): |
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.
When will users pass an ExperimentLogger via loggers
?
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.
They shouldn', but they might, so I guess we should handle this. We could also raise an error here instead
has_csv_logger = True | ||
if JsonLogger in callback.logger_classes: | ||
has_json_logger = True | ||
# Todo(krfricke): add checks for new ExperimentLogger classes |
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.
will this todo be part of this PR?
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.
No, this will be a follow up once we implemented a "native" CSV/JSON ExperimentLogger
if syncer_index is not None and last_logger_index is not None and \ | ||
syncer_index < last_logger_index: | ||
if (not has_csv_logger or not has_json_logger) and not loggers: | ||
# Only raise the warning if the loggers were passed by the user. | ||
# (I.e. don't warn if this was automatic behavior and they only | ||
# passed a customer SyncerCallback). | ||
raise ValueError( | ||
"The `SyncerCallback` you passed to `tune.run()` came before " | ||
"at least one `ExperimentLogger`. Syncing should be done " | ||
"after writing logs. Please re-order the callbacks so that " | ||
"the `SyncerCallback` comes after any `ExperimentLogger`.") |
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.
hmm, can we just make sure the syncer is last?
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.
That's what we do here, right? If someone passes callbacks=[syncer, logger]
it will raise an error. If someone passes callbacks=[syncer]
and the logger is created automatically, it will just put the syncer last and everything will work.
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.
looks good, after nits
Why are these changes needed?
Second split of PR ##11699, builds on #11748
Checks
scripts/format.sh
to lint the changes in this PR.