GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-120868: Fix breaking change in `logging.config` when using `QueueHandler` by provinzkraut · Pull Request #120872 · python/cpython Conversation
|
| # if it's not an instance of BaseProxy, it also can't be | ||
| # an instance of Manager.Queue / Manager.JoinableQueue | ||
| if isinstance(qspec, BaseProxy): | ||
| proxy_queue = MM().Queue() |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
If there is an error with the creation, you could maybe wrap that one in an try-except block? (probably catching an OSError but I'm not sure).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Would we want to behave different in this case? I'm not sure what the expected behaviour would be here. The only thing I could think of that would make sense is to raise the usual TypeError from the error the instantiation raised, but that would still result in roughly the same traceback as when calling .configure, since it already re-raises exceptions.
Or are you suggesting we take a different path here when we encounter an error?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
No, I didn't know about the exceptino being wrapped by configure so it may be fine. The thing is, I don't know whether we could be more precise in the error being raised so that you know that's is because multiprocessing queues are not supported.
For instance:
MM().Queue() is fine -> you want the TypeErrorMM().Queue() with some tweaks. In this case, you will be able to create your special queue object and expect it to work but MM().Queue() would fail. So instead of having some generic error due to that instantiation, the message could be something else. But I'm not sure whether this corner case is too convoluted or not.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I think, I'll just leave the decision to Vinay because they're the maintainer and creator of the logging module.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
But I'm not sure whether this corner case is too convoluted or not.
Yeah, I was worrying about that. I didn't want to put too much stuff in here just to handle this case. IMO the proper proper fix for this would be to add some level of introspection capabilities in BaseProxy, that allows to perform such checks without creating an instance of the type you want to check against first.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would make sense to add something in the docs for this, as there's quite a few special cases now that might not be immediately intuitive
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| q = self.configure_custom(dict(qspec)) | ||
| else: | ||
|
|
||
| if isinstance(qspec, str): |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
"Flat is better than nested", so I agree that it's reasonable to rearrange the various tests on qspec in this way.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
This reverts commit afcc79e.
|
Thanks for the review @vsajip! I have made the requested changes; please review again :) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks for making the requested changes! @vsajip: please review the changes made to this pull request. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
🤖 New build scheduled with the buildbot fleet by @vsajip for commit 6daf85e 🤖 If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Some buildbot failures, but they appear unrelated to this change.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks @provinzkraut for the PR, and @vsajip for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks @provinzkraut for the PR, and @vsajip for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…QueueHandler` (pythonGH-120872) (cherry picked from commit 7d9c685) Co-authored-by: Janek Nouvertné <[email protected]>
…QueueHandler` (pythonGH-120872) (cherry picked from commit 7d9c685) Co-authored-by: Janek Nouvertné <[email protected]>
|
GH-121077 is a backport of this pull request to the 3.12 branch. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
GH-121078 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.