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
Use consistent logging #1611
Use consistent logging #1611
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1611 +/- ##
==========================================
+ Coverage 79.94% 79.97% +0.03%
==========================================
Files 293 293
Lines 14634 14669 +35
==========================================
+ Hits 11699 11732 +33
- Misses 2935 2937 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -184,33 +182,32 @@ def _verify_version(self): | |||
parsed_version = version.parse(self.client_lib_version) | |||
|
|||
if self._client_config.min_compatible_version and self._client_config.min_compatible_version > parsed_version: | |||
click.echo( | |||
styled_msg = click.style( |
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.
Shouldn't we also call logger.error(styled_msg)
right after that?
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.
this change was removed as its part of legacy client
89e7484
to
ab28e41
Compare
7deafa4
to
ab66632
Compare
ab66632
to
72ff56f
Compare
Sample output after changes: [neptune] [info ] Neptune initialized. Monitor the logging in the app: https://app.neptune.ai/rafal.neptune/test/e/TES-1274
[neptune] [warning] Info (NVML): NVML Shared Library Not Found. GPU usage metrics may not be reported. For more information, see https://docs.neptune.ai/help/nvml_error/
[neptune] [info ] Shutting down background jobs, please wait a moment...
[neptune] [info ] Done!
[neptune] [info ] Waiting for the remaining 18 operations to synchronize with Neptune. Do not kill this process.
[neptune] [info ] All 18 operations synced, thanks for waiting!
[neptune] [info ] Explore the metadata in the Neptune app: https://app.neptune.ai/rafal.neptune/test/e/TES-1274/metadata
[neptune] [info ] Neptune initialized. Monitor the logging in the app: https://app.neptune.ai/rafal.neptune/test/e/TES-1275
[neptune] [info ] Shutting down background jobs, please wait a moment...
[neptune] [info ] Done!
[neptune] [info ] Waiting for the remaining 18 operations to synchronize with Neptune. Do not kill this process.
[neptune] [info ] All 18 operations synced, thanks for waiting!
[neptune] [info ] Explore the metadata in the Neptune app: https://app.neptune.ai/rafal.neptune/test/e/TES-1275/metadata |
warning = ( | ||
"Info (NVML): %s. GPU usage metrics may not be reported. For more information, " | ||
"see https://docs-legacy.neptune.ai/logging-and-managing-experiment-results" | ||
"/logging-experiment" | ||
"-data.html#hardware-consumption " | ||
) |
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.
Under which circumstances does this show? Just wondering whether we should update the ancient docs link.
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.
This looks to be quite consistent with the circumstances etc. and up-to-date: https://docs.neptune.ai/help/nvml_error/ ?
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.
Yep! And if the page is not up to snuff, I'll fix it 😉
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.
LGTM
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.
E2E tests failed (see link above). This will need to be addressed before merging
- added common logger class - added logging setup and its loading - updated unit tests - TODO: verify, check if no redundant changes are present, add test to check if loggers are logging with common prefix
…nge log format to only desplay log message
Co-authored-by: Sabine <sabine.nyholm@neptune.ai>
Co-authored-by: Sabine <sabine.nyholm@neptune.ai>
12a7570
to
4c70d32
Compare
E2E one more time: https://github.com/neptune-ai/neptune-client/actions/runs/7713014109 |
Before submitting checklist
Did you ask the docs owner to review all the user-facing changes?