Skip to content

Adds logging [INFO] to TimelineVisualizationCallback and HistoryCallback #548

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

Merged
merged 6 commits into from
Aug 13, 2024

Conversation

norlandrhagen
Copy link
Contributor

Small PR that:

  • Adds info level logging to show the directory that TimelineVisualizationCallback and HistoryCallback write their artifacts to.
  • Updates the TimelineVisualizationCallback os.mkdir to the Pathlib mkdir used in the HistoryCallback. This seems to fix the FileNotFoundError in TimelineVisualizationCallback() usage [cubed - cubed/xarray] #544.

example logging:

TimelineVisualizationCallback results saved to directory: history/compute-20240808T135430.657101/
HistoryCallback results saved to directory: history/compute-20240808T135430.657101/

Some remaining Q's:

  • The TimelineVisualizationCallback calls an external function (to the class) create_timeline that has args for such as dst=None, which are not a part of TimelineVisualizationCallback. Is this create_timeline function used anywhere else? I ask this b/c the optional dst arg creates a few more path creation options. If it is confined to TimelineVisualizationCallback, the logic could be more similar to HistoryCallback which is a bit cleaner.

  • I added info level logging to diagnostics/__init__.py is this OK to set logging here?

@TomNicholas
Copy link
Member

Nice! Would this close either of #543 or #544?

@norlandrhagen
Copy link
Contributor Author

I think it should close both!

@norlandrhagen
Copy link
Contributor Author

No idea why the Apache Beam tests are failing though 😕

@tomwhite
Copy link
Member

tomwhite commented Aug 9, 2024

Thanks for the PR @norlandrhagen!

  • The TimelineVisualizationCallback calls an external function (to the class) create_timeline that has args for such as dst=None, which are not a part of TimelineVisualizationCallback. Is this create_timeline function used anywhere else? I ask this b/c the optional dst arg creates a few more path creation options. If it is confined to TimelineVisualizationCallback, the logic could be more similar to HistoryCallback which is a bit cleaner.

This code was originally based on code from Lithops for producing a timeline image, so some of it may be redundant. Please go ahead and remove code paths that are not used so it's more similar to HistoryCallback.

  • I added info level logging to diagnostics/__init__.py is this OK to set logging here?

No, it should go in client code. Perhaps add it to examples/add-random.py and examples/matmul-random.py?

@tomwhite
Copy link
Member

tomwhite commented Aug 9, 2024

No idea why the Apache Beam tests are failing though 😕

Not related to this PR. Opened #549

…timeline(), removed default logging info level from __init__
@norlandrhagen
Copy link
Contributor Author

  • I added info level logging to diagnostics/__init__.py is this OK to set logging here?

No, it should go in client code. Perhaps add it to examples/add-random.py and examples/matmul-random.py?

Do you see any place where logging or optional logging might be useful? Maybe default off, but user enabled --debug flag could output some info about where callback artifacts are written. Also happy to remove it and just mention in the docs of where you can expect them to be written.

ex:

TimelineVisualizationCallback results saved to directory: history/compute-20240809T115943.795332/timeline.svg/
HistoryCallback results saved to directory: history/compute-20240809T115943.795332/

@tomwhite
Copy link
Member

It would be useful, but the problem is that if you configure logging then that will interfere with logging that the user has configured (often in non-obvious ways). So I think adding logging to the examples is a good compromise. Also mentioning in the docs.

@norlandrhagen
Copy link
Contributor Author

Makes sense! Thanks for explaining.

Copy link
Member

@tomwhite tomwhite left a comment

Choose a reason for hiding this comment

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

Thanks @norlandrhagen! LGTM. I've made one small suggestion. Let me know when you're ready to merge.

@norlandrhagen
Copy link
Contributor Author

If you're happy with the doc suggestions, then it's ready to merge.

@tomwhite tomwhite merged commit 3dbbc9a into cubed-dev:main Aug 13, 2024
10 of 11 checks passed
thodson-usgs pushed a commit to thodson-usgs/cubed that referenced this pull request Oct 24, 2024
…llback` (cubed-dev#548)

* added info level logging to HistoryCallback to share directory

* add logging to timeline & update mkdir to pathlib style

* moved figure creation into timeline on_compute_end(), removed create_timeline(), removed default logging info level from __init__

* removed history  & timeline logging

* adds more detail to diagnostics.md

* Update docs/user-guide/diagnostics.md

Co-authored-by: Tom White <[email protected]>

---------

Co-authored-by: Tom White <[email protected]>
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.

3 participants