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

Check that logfile for manager's logger doesn't collide with manager's own logfile #215

Merged
merged 11 commits into from
Feb 20, 2024

Conversation

vreuter
Copy link
Member

@vreuter vreuter commented Feb 18, 2024

Close #212
Close #211

@vreuter
Copy link
Member Author

vreuter commented Feb 18, 2024

IDK why lint action is failing; the 2 files it alleges in the logs that would be reformatted haven't changed since the last run, for version 0.14 release. Maybe something w/ black's Github Action changed?

When I just run black . locally, all's good.

@donaldcampbelljr
Copy link
Member

Try upgrading your local black. I had this issue happen to me a couple of weeks ago and I seem to remember that upgrading caused the file to be formatted differently (and finally matched the github action).

@vreuter
Copy link
Member Author

vreuter commented Feb 18, 2024

Try upgrading your local black. I had this issue happen to me a couple of weeks ago and I seem to remember that upgrading caused the file to be formatted differently (and finally matched the github action).

TY @donaldcampbelljr , indeed that works!

@vreuter
Copy link
Member Author

vreuter commented Feb 19, 2024

@nsheff if you review this also, note that the only real changes are to tests and to manager.py; the other files in the diff are formatter-related.

@vreuter
Copy link
Member Author

vreuter commented Feb 20, 2024

@donaldcampbelljr OK to merge to master, or you want me to switch the target to dev?

@donaldcampbelljr
Copy link
Member

Oh lets do dev for now, please.

@nsheff
Copy link
Member

nsheff commented Feb 20, 2024

Just a broader question: should we just disable the logmuse handle to change the log files in the first place?

I mean, there are no logs coming out of pypiper that logmuse should be touching since it has its own logging system, right?

Or did I miss something?

@vreuter
Copy link
Member Author

vreuter commented Feb 20, 2024

there are no logs coming out of pypiper that logmuse should be touching since it has its own logging system, right?

@nsheff sorry I don't understand this, what do you mean here exactly? What do you mean by "coming out of pyiper" and "should be touching"?

@vreuter vreuter changed the base branch from master to dev February 20, 2024 17:24
@vreuter
Copy link
Member Author

vreuter commented Feb 20, 2024

Oh lets do dev for now, please.

👌 , done :)

@vreuter
Copy link
Member Author

vreuter commented Feb 20, 2024

Just a broader question: should we just disable the logmuse handle to change the log files in the first place?

I mean, there are no logs coming out of pypiper that logmuse should be touching since it has its own logging system, right?

Or did I miss something?

A couple reasons that come to mind to have a separate one...
a) If I write a manager subclass, I can use the logger for things about the manager while reserving the pipeline log, conceptually, for things about the pipeline
b) I can get a logfile that's much less verbose, e.g. setting the level for the logger's logfile to warning, while the pipeline log will tend to get many more messages.

@nsheff
Copy link
Member

nsheff commented Feb 20, 2024

To me, I think there's really only one set of logs: the logs from the pipeline. Everything that should be output by a PipelineManager is the pipeline's logs.

So, I don't see these as separate, and so I've never used logmuse to configure logs on something that uses pypiper, and that's why this has never been a problem.

Do you see an actual use case where you want some logs to come from the Manager and some other logs to come from the Pipeline? I can't imagine such a situation, but if you think you would use it that way, then this is fine with me! I think I view the Manager object as somehow synonymous with the pipeline itself, so I don't understand why someone would want that level of control.

Copy link
Member

@nsheff nsheff left a comment

Choose a reason for hiding this comment

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

looks good! thanks for fixing this

@vreuter
Copy link
Member Author

vreuter commented Feb 20, 2024

Do you see an actual use case where you want some logs to come from the Manager and some other logs to come from the Pipeline? I can't imagine such a situation, but if you think you would use it that way, then this is fine with me! I think I view the Manager object as somehow synonymous with the pipeline itself, so I don't understand why someone would want that level of control.

I sort of see both sides. Personally, I can definitely see using the logfile for the manager's logger to output more technical details and/or using it for only more severe messages, so having the flexibility both in terms of verbosity (e.g., I may want to go through a logfile that has nothing from any of the 3rd-party tools my pipeline may be using, and maybe I don't want to look at certain types of noisy numpy warnings) and in terms of nature/subject of message (e.g., mechanics of pipeline to logger, actual bioinformatics work to to pypiper log).

But I understand the appeal of the simplicity of the single log destination, too. If starting from scratch, I'd probably be inclined to favor the single logfile in fact. But since the logger's in place, I'd like to keep it for now. We could revisit as a future refactor the consolidation/merging of logging destinations.

@nsheff would you like me to open a ticket with that as a refactor idea?

@vreuter vreuter merged commit 59e1afb into dev Feb 20, 2024
3 checks passed
@vreuter vreuter deleted the vr/logfile-212 branch May 5, 2024 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants