-
Notifications
You must be signed in to change notification settings - Fork 9
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
Conversation
…atment of arguments across branches
IDK why When I just run |
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! |
@nsheff if you review this also, note that the only real changes are to |
@donaldcampbelljr OK to merge to |
Oh lets do |
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? |
@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"? |
👌 , done :) |
A couple reasons that come to mind to have a separate one... |
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. |
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! thanks for fixing this
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? |
Close #212
Close #211