-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Make indentation work with multiple threads #4654
Conversation
As noted on both the issues referenced, pip does not support in-process usage, so this PR, while not particularly problematic in itself, isn't actually needed in practice. |
Agreed if pip needs the functionality internally this will help. And agreed there's no real harm in the change. I have a (mild) concern that if we fix this issue, the people encountering #3889 and #2553 could hit further issues and expect support - but we can cross that bridge when we come to it. So overall I'm neutral on this change. |
@pfmoore Currently, the issues you linked to provide a good description of why the errors come up and what the underlying reasoning for them is... Crossing this bridge (merging this PR) would mean that there would be more issues from people stumbling over new, different, problems. People have worked around those issues and hit other problems anyway - #3889 (comment). My inputs on this would be that we should defer this change until pip actually gets a supported API. |
@pradyunsg Thanks for finding the reference to the follow-up problem. Yes, given that we know there are other threading issues lurking behind this one, that switches me to a definite -1 on merging this. Regarding the issues that prompted this, I'd specifically state that pip does not support being used in the presence of threads, and we've no plans to work on making pip thread-safe at this time. (Any work we do on parallel downloads or similar will only ensure that the specific code we're changing is thread-safe, so that doesn't alter the situation). |
As mentioned in the initial comment in this PR, we are using this logging code also outside of pip. We like indentation and colored output, so for use it as utils. The code which uses pip is not threaded in our case. But once we have dependencies in place (for what we use pip), we use threads, and logging. Logging is thread safe, so it is a surprise that this helper function is not. I know that this is maybe not main use case of pip, to be utility library for other apps, but because we already have it as a dependency, and it does pretty good job with this logging, we would like to use it. And this PR would make it work for us very well. |
Feel free top copy the code if you find it useful, or maintain a patched version locally. But we won't be adding this to pip, sorry. |
When using
indent_log
log with multiple threads (like subprocessing) thenindent_log
does not really work in other threads because_log_state.indentation
is initialized only in initial thread. Importing module again does not work either (to initialize the value in the new thread), because information about which modules have been imported is shared between threads.In our case we use
pip.utils.logging.indent_log
outside of just pip, because we found it useful.This check makes sure that value is always initialized.
See #3889 and #2553 for more information.