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.

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

creates project specific logger #233

Merged
merged 3 commits into from
Jul 11, 2023
Merged

creates project specific logger #233

merged 3 commits into from
Jul 11, 2023

Conversation

fredshone
Copy link
Contributor

fixes #150 by changing to a "project specific logger" in main, rather than calling the default. This prevents setting the underlying dependancy loggers to debug. I have tested locally.

I've also changed the config key from verbose to debug. I feel this more accurately reflects the option. Old configs with verbose = true will still work but elara will silently revert to debug = false. Normally i would think this is dodgy, but in this case I think it is appropriate.

Docs and examples updated.

@fredshone fredshone requested review from mfitz and syhwawa June 23, 2023 12:31
@@ -3,7 +3,8 @@ name = "test_town"
time_periods = 24
scale_factor = 0.0001
crs = "EPSG:27700"
verbose = false
debug = false
verbose = true
Copy link
Contributor

Choose a reason for hiding this comment

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

In the fixed code, Will debug = false verbose = true in the config be equivalent to debug = false by setting level to debug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I treat verbose as synonymous with debug (perhaps more like "debug mode"). So verbose = false same as debug = false and vice versa.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I change to "debug" because the expectation is that you would set this to false unless you are actually debugging. Whereas "verbose" sounded more casual.

Basically i expect this to nearly always be false.

Copy link
Contributor

@syhwawa syhwawa left a comment

Choose a reason for hiding this comment

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

Nice job!

I am wondering if it will be easy to use the existing function here to set different logging levels without introducing two parameters?

For example, verbose=info representing the loggings will be on INFO level
verbose=debug representing the loggings will be on DEBUG level

Copy link
Contributor

@mfitz mfitz left a comment

Choose a reason for hiding this comment

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

The logging config change looks fine, although it would be nice to see more evidence of how you tested it locally and what happened.

On a different note - this is a user-facing change. If I'm understanding correctly, it's backwards compatible rather than breaking, but it's still a change that affects the user. As such, I think this is a minor version bump. Just looking at the current version: __version__ = "0.0.0". I think this moves it to "0.1.0".

Now might be a good time to create a CHANGELOG.md too (see here for a template), and to create a GitHub release.

@fredshone
Copy link
Contributor Author

Nice job!

I am wondering if it will be easy to use the existing function here to set different logging levels without introducing two parameters?

For example, verbose=info representing the loggings will be on INFO level verbose=debug representing the loggings will be on DEBUG level

well spotted! I changed the docs to push people to use either true or false. but it will also support specific levels. But I don't think really need to expose this to anyone.

@fredshone
Copy link
Contributor Author

The logging config change looks fine, although it would be nice to see more evidence of how you tested it locally and what happened.

On a different note - this is a user-facing change. If I'm understanding correctly, it's backwards compatible rather than breaking, but it's still a change that affects the user. As such, I think this is a minor version bump. Just looking at the current version: __version__ = "0.0.0". I think this moves it to "0.1.0".

Now might be a good time to create a CHANGELOG.md too (see here for a template), and to create a GitHub release.

I tested by creating a "verbose" example config then checked to see if the "eroneous" logging is removed in the new branch. I say "locally" because as noted in the #150 behaviour does seem to vary my environment, most likely due to loose dependancies or sketch dev installs 🦡 .

So happy if others want to check.

This PR will silently make all existing state machine configs "quiet", which means we might be hiding weird behaviour on box. But the expectation is that no one will be running debug on a box in any case. Certainly not "unsupervised".

I have added Change log but then removed release note - I do not want the duplication on a project that is entering retirement.

@fredshone
Copy link
Contributor Author

Since also tested on our infra. Merging

@fredshone fredshone merged commit db721ae into main Jul 11, 2023
1 check passed
@fredshone fredshone deleted the reduce-debug-logging branch July 11, 2023 10:51
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.

Setting Logging Level to DEBUG includes Matplotlib logging
3 participants