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

Fix smart_open deprecation warning globally #2530

Merged
merged 3 commits into from
Jul 7, 2019

Conversation

itayB
Copy link
Contributor

@itayB itayB commented Jun 15, 2019

Continued from #2528.

Fixes #2531, closes #2528.

Copy link
Owner

@piskvorky piskvorky left a comment

Choose a reason for hiding this comment

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

This is too wild. The upgrade will need a finer comb, more attention to particular instances, than this mechanical substitution.

gensim/corpora/_mmreader.c Outdated Show resolved Hide resolved
gensim/corpora/_mmreader.pyx Outdated Show resolved Hide resolved
gensim/utils.py Outdated Show resolved Hide resolved
@mpenkov
Copy link
Collaborator

mpenkov commented Jun 15, 2019

I agree with @piskvorky that blindly find-replacing is not the way to go here. Let's come up with a more intelligent approach.

You'll see most code accesses smart_open indirectly via the utils module. Perhaps we can contain the changes to that module? That will reduce the risk and make things easier to test.

@itayB
Copy link
Contributor Author

itayB commented Jun 15, 2019

I can create smart_open function in utils module. This function will call to the new open so we won't have to change in so much places. WDYT?
I will try to do later on (I'm out now)

@piskvorky
Copy link
Owner

piskvorky commented Jun 15, 2019

Nah, I'm for replacing it everywhere, consistently, not only in utils. Just take more care with the migration: not every smart_open string ought to become open, plus there's the r vs rb default mode.

I don't think the changeset will be so difficult to warrant some new wrappers. Most (all?) smart_open instances should be pretty straightforward to migrate, at-a-glance.

@itayB
Copy link
Contributor Author

itayB commented Jun 15, 2019

OK, I'll fix it later on and keep you posted

gensim/utils.py Outdated Show resolved Hide resolved
gensim/utils.py Outdated Show resolved Hide resolved
gensim/models/poincare.py Outdated Show resolved Hide resolved
@itayB
Copy link
Contributor Author

itayB commented Jun 22, 2019

@mpenkov @piskvorky is there anything else you want me to do in this PR?

@piskvorky
Copy link
Owner

Thanks a lot @itayB , let's wait for @mpenkov 's review.

Copy link
Collaborator

@mpenkov mpenkov left a comment

Choose a reason for hiding this comment

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

We're still inconsistent about opening files. I think it's worth consolidating before we proceed with the PR.

gensim/models/deprecated/old_saveload.py Outdated Show resolved Hide resolved
gensim/models/doc2vec.py Outdated Show resolved Hide resolved
@mpenkov
Copy link
Collaborator

mpenkov commented Jun 28, 2019

In the future, please avoid squashing everything together and force-pushing during a review. It makes it difficult for the reviewer to manage the change set. Right now, it's impossible to distinguish code that I've reviewed already, and new code that you've pushed since the previous review.

@itayB
Copy link
Contributor Author

itayB commented Jun 28, 2019

In the future, please avoid squashing everything together and force-pushing during a review. It makes it difficult for the reviewer to manage the change set. Right now, it's impossible to distinguish code that I've reviewed already, and new code that you've pushed since the previous review.

No problem, sorry about that

@mpenkov
Copy link
Collaborator

mpenkov commented Jun 28, 2019

OK, I think this is ready to merge. @piskvorky Do you have any objections?

@piskvorky
Copy link
Owner

No objections, nice work @itayB ! This Gensim cleanup was long overdue.

@itayB
Copy link
Contributor Author

itayB commented Jun 28, 2019

No objections, nice work @itayB ! This Gensim cleanup was long overdue.

thanks @piskvorky !
I think that there's still 1 change requested that is waiting for your approval :-)

@itayB
Copy link
Contributor Author

itayB commented Jul 2, 2019

Hi @mpenkov @piskvorky, I just want to make sure; Is there anything that you expect me to do or should I just wait for you to merge this?

@mpenkov
Copy link
Collaborator

mpenkov commented Jul 3, 2019

Please wait for us to merge. Thank you for your contribution and your patience.

@mpenkov mpenkov merged commit 27bbb70 into piskvorky:develop Jul 7, 2019
@mpenkov
Copy link
Collaborator

mpenkov commented Jul 7, 2019

OK, merged. @itayB Thank you for taking care of this!

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.

MmWriter raises UserWarning on __init__
3 participants