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

allow passing loggers into some databasing classes for working with prefect #563

Closed
wants to merge 5 commits into from

Conversation

guanyilun
Copy link
Contributor

The issue with logging in prefect is that, although prefect loggers are passed in as an argument to the flow, they didn't get passed all the way into each class that does the actual work such as G3tsmurf or Imprinter classes. This PR is a step towards fixing it.

@mhasself
Copy link
Member

I am very uncomfortable with having to propagate an external logger to every place where we might want to capture output from a script. This is practically disqualifying, in my mind. (I realized the other day that it was a very bad sign that we needed to pass in a logger to our entry points...)

Does the feature described here help us at all? I really think we need a system that captures stdout/stderr without having to propagate prefecty objects inside our pipeline code. If we have to write our own wrappers to capture and pipe out the output, that is more appealing to me than having to tunnel an external logger deep into our libraries.

@guanyilun
Copy link
Contributor Author

ok that's a fair point. I can experiment with that

@kmharrington
Copy link
Member

what's the status of this? has it been supplanted?

@guanyilun
Copy link
Contributor Author

this is no longer being worked on. We used a different logging solution, as pointed out by Matthew.

@mhasself
Copy link
Member

this is no longer being worked on. We used a different logging solution, as pointed out by Matthew.

I don't recall learning that!

Does that mean we don't have to pass "logger" in anymore? Because that would make the site_pipeline scripts a lot easier to write.

@guanyilun
Copy link
Contributor Author

yes, I think we can plan to phase it out

@kmharrington
Copy link
Member

right now the setup has things still throwing errors if logger isn't the last kwarg. what is it that's requiring that?

@guanyilun
Copy link
Contributor Author

logger no longer required

@guanyilun guanyilun closed this Mar 13, 2024
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