Skip to content

Added with context manager to if clauses in _monkey_patch #19367

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

Merged
merged 3 commits into from
Feb 6, 2021

Conversation

mjkanji
Copy link
Contributor

@mjkanji mjkanji commented Feb 6, 2021

Reference Issues/PRs

References #19349

What does this implement/fix? Explain your changes.

Added a with open() context manager when reading files in the _monkey_patch_webbased_functions function.

Any other comments?

Reduces the number of failed tests from 33 to 16.

@mjkanji
Copy link
Contributor Author

mjkanji commented Feb 6, 2021

@ogrisel Please take a look. The number of failed tests is down to 10, but now, instead of a ResourceWarning, I'm getting a

UserWarning: Multiple active versions of the dataset matching the name cpu exist. Versions may be fundamentally different, returning version 1.

@ogrisel
Copy link
Member

ogrisel commented Feb 6, 2021

The UserWarning is unrelated to the original problem. So let's open a dedicated PR for this specific warning.

@reshamas
Copy link
Member

reshamas commented Feb 6, 2021

#DataUmbrella

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM. Let's see if the continuous integration is happier.

@ogrisel ogrisel changed the title [WIP] Added with context manager to if clauses in _monkey_patch Added with context manager to if clauses in _monkey_patch Feb 6, 2021
@glemaitre glemaitre merged commit 70534d6 into scikit-learn:main Feb 6, 2021
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mjkanji mjkanji deleted the openml_resourcewarning branch February 6, 2021 09:22
@glemaitre glemaitre added this to the 0.24.2 milestone Feb 11, 2021
@glemaitre glemaitre added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label Feb 11, 2021
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
12 tasks
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:datasets To backport PR merged in master that need a backport to a release branch defined based on the milestone.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants