Add test and fix stub for sklearn.config_context#3155
Conversation
|
Hi @fhoehle! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
rchen152
left a comment
There was a problem hiding this comment.
Thanks for the PR! Pyrefly's bundled sklearn stubs are mirrored from here:
. So rather than editing our bundled copy, it would be better to fix the stubs upstream and then bundle a fresh copy.(If you get pushback on or lack of response to an upstream PR, we can consider doing a local patch instead. We'd need to record the patch somewhere, so we know what we've modified.)
|
Many thanks for pointing out the mirroring I oversaw it. After updating the upstream source what are the concrete steps to propagate the change? The |
|
@jvansch1 originally added the bundled stubs and can correct me if I'm wrong, but I believe we just manually copied them over. |
|
This pull request has been automatically marked as stale because it has not had recent activity for more than 2 weeks. If you are still working on this this pull request, please add a comment or push new commits to keep it active. Otherwise, please unassign yourself and allow someone else to take over. Thank you for your contributions! |
|
Current status: Waiting for an upstream maintainer to review the proposed changes. |
|
@fhoehle Thank you for opening a PR with microsoft/python-type-stubs. It looks like the repo may be unmaintained now, which is unfortunate. @jvansch1 Do you have any thoughts on what to do here? I was originally thinking that if we diverge from the upstream stubs, we should maintain a stack of patches to make it easier to cleanly reapply our changes on top of a new version, but maybe that's unnecessary if the original repo is inactive. |
|
@rchen152 I opened an upstream issue asking about maintainership and also pinged a Pylance maintainer who handles their releases. I'd propose we wait a little to see if anyone responds and clarifies the maintenance. |
Summary: Refresh the sklearn stubs to the upstream commit, fixing `config_context` to type-check as a context manager and adding the `"polars"` option to `set_output`. Add basic tests covering the sklearn stubs.
dcf6daf to
06b4b06
Compare
|
Hi @rchen152 — the upstream PR is now merged, so I've bumped the vendored |
Summary
This PR adds a new test asserting that the
sklearn.config_contextcontext manager is correctly understood.The initial failing test is fixed by correcting the corresponding
sklearn.config_contexttype hint and adding thecontextlib.contextmanagerdecorator.Issue #3154 describes the observed error. The originating cause is the incorrect
sklearn-stubin thepyrefly_bundledcrate.The deprecation warning regarding
Iterator[None]is mitigated by replacing it withGeneratorTest Plan
03b707d7bb6aeb55d9c5f2b6dd7b6e7367fe43a4, which failed with the above-describedbad-context-managererror.cargo test -p pyrefly --lib test_sklearntest.py