Replace deprecated smart_open.smart_open function with a thin wrapper #544

mpenkov · 2020-10-01T11:57:26Z

We're about to do a major version bump, so it's a good chance to drop deprecated functionality.

piskvorky · 2020-10-01T12:12:01Z

Scary. Any harm keeping smart_open.smart_open as an alias of smart_open.open?

This will break a lot of code… including our own in gensim-data.

mpenkov · 2020-10-01T12:20:39Z

Scary. Any harm keeping smart_open.smart_open as an alias of smart_open.open?

Yeah, the harm is that they're not strictly compatible - the former passes parameters as *args, the latter wraps them in a dict.

This will break a lot of code… including our own in gensim-data.

We could either pin the version, or stop using the deprecated function. It's been deprecated for years now.

piskvorky · 2020-10-01T12:32:00Z

Yeah, we'll have to re-release the datasets. They're meant to be immutable, so can't (=shouldn't) edit.

piskvorky · 2020-10-01T12:36:08Z

It's been deprecated for years now.

Less than a year (Nov 2019).

The smart_open README looks broken / unfinished; can you have a look?

Version 2.0 will introduce a backwards incompatible installation method with regards to the cloud dependencies. A migration path to minimize breaking was introduced in version x.x.x. If you want to maintain backwards compatibility (installing all dependencies) install this package via smart_open[all] now and once the change is made you should not have any issues. If all you care about is AWS dependencies for example you can install via smart_open[s3] and once the dependency change is made you will simply drop the unwanted dependencies. You can read more about the motivations here

mpenkov · 2020-10-01T12:48:35Z

A migration path to minimize breaking was introduced in version x.x.x.

Let's take a moment to truly appreciate this prophetically ironic sentence...

krzemienski · 2020-10-01T19:20:37Z

A migration path to minimize breaking was introduced in version x.x.x.

Let's take a moment to truly appreciate this prophetically ironic sentence...

Or maybe another moment to wonder just how smart smart_open is? Glad I’ve learned to watch repos I use in production code!

mpenkov · 2020-10-02T03:03:59Z

Yeah, we'll have to re-release the datasets. They're meant to be immutable, so can't (=shouldn't) edit.

If we're gonna open up that can of worms, then it may be worth thinking about how we handle gensim-data. As @gojomo and @menshikh-iv pointed out, the current approach has several shortcomings:

Code accompanying each dataset is not in version control. It's an __init__.py file attached to each release.
There is no dependency control. If the dataset depends on a package that isn't already installed, or touches a part of gensim that no longer exists, it isn't possible to install it.
There is a security risk because gensim downloads a Python script from a network location and then executes it.

While most of the above is for us to work out as part of the gensim effort (e.g. piskvorky/gensim#2283), the relevance to smart_open is as follows:

We cannot easily locate datasets that use the old deprecated smart_open.smart_open function. We have to go through each release and eyeball the __init__.py files. This is a bad sign.
We cannot pin the smart_open version for each dataset.

@piskvorky WDYT about the above points? How shall we move forward, both with this PR and in general?

gojomo · 2020-10-02T03:20:54Z

One way to still-soften the removal would be to leave a stub that errors with a message pointing to help (or if possible, the exact recommended line-of-code with which to replace old usages). Compared to just a cannot import name 'smart_open' error on import, or a has no attribute 'smart_open' error on a smart_open.smart_open() usage, it will save those hitting it some Googling, and could also mention "function was deprecated in x.x.x [ha] and is now removed".

For gensim-data, I'd recommend it become its own stub PyPI package, with versioned releases, & any code that ever executes/downloads other large out-of-package data in source control. Adding a new named dataset would require a new release, which users would pip install --upgrade gensim-data so their download-stubs know the new dataset name/remotes.

piskvorky · 2020-10-02T09:59:44Z

Gensim-data is low priority for me right now (and has been for a long time). I'd probably design it differently (and not call it api – what API?), but I'd still keep the immutability of a resource release/version (of both code and data), that's vital. The "code" part can be handled by standard packaging.

This came to my attention only because I saw deprecations coming from a dataset that relies on smart_open.open, which will trigger a hard fail after this PR. Other than that I have no bandwidth for gensim-data atm, sorry.

@piskvorky WDYT about the above points? How shall we move forward, both with this PR and in general?

Fix README + keep smart_open.smart_open. I understand it's incompatible (params in args vs dict) so an alias is risky, so probably an error stub like @gojomo says.

gojomo · 2020-10-03T10:11:24Z

smart_open/smart_open_lib.py

+            'for more information.' % sorted(kwargs)
+        )
+    del kwargs
+    return open(**locals())


If the actual end goal is true deprecation/removal of the function itself - not just some sets of supported parameters – perhaps OK to fail in this case, too. (Or at least: show some deprecation warning before proceeding?) However, if it's true that in this case, the caller can just use smart_open.open() with exactly the same params instead, the message could assure them that.

We can't fail in this case because the gensim-data stuff will break.

I agree with the other suggestion.

piskvorky

Clever solution! I like it.

How about the incomplete README?

smart_open/smart_open_lib.py

mpenkov added 2 commits October 1, 2020 20:54

remove deprecated smart_open function

6241320

git rm smart_open/tests/test_smart_open_old.py

554e105

mpenkov requested a review from piskvorky October 1, 2020 11:57

mpenkov changed the title ~~Delete deprecated smart_open.samrt_open function~~ Delete deprecated smart_open.smart_open function Oct 1, 2020

add a thin wrapper for backward compatibility

fb36d0e

gojomo reviewed Oct 3, 2020

View reviewed changes

piskvorky approved these changes Oct 3, 2020

View reviewed changes

smart_open/smart_open_lib.py Outdated Show resolved Hide resolved

mpenkov added 6 commits October 3, 2020 22:21

update tests

923ceef

fix readme

addfb01

adjust wrapper

3c6542d

add test for wrapper

e129f19

remove unused import

0d7ef86

Py3.5, argh

95463be

mpenkov changed the title ~~Delete deprecated smart_open.smart_open function~~ Replace deprecated smart_open.smart_open function with a thin wrapper Oct 4, 2020

Update README.rst

92163ff

mpenkov merged commit d03bdeb into develop Oct 8, 2020

mpenkov deleted the rm_smart_open branch October 8, 2020 08:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace deprecated smart_open.smart_open function with a thin wrapper #544

Replace deprecated smart_open.smart_open function with a thin wrapper #544

mpenkov commented Oct 1, 2020

piskvorky commented Oct 1, 2020 •

edited

Loading

mpenkov commented Oct 1, 2020 •

edited

Loading

piskvorky commented Oct 1, 2020 •

edited

Loading

piskvorky commented Oct 1, 2020 •

edited

Loading

mpenkov commented Oct 1, 2020

krzemienski commented Oct 1, 2020 •

edited

Loading

mpenkov commented Oct 2, 2020

gojomo commented Oct 2, 2020

piskvorky commented Oct 2, 2020 •

edited

Loading

gojomo Oct 3, 2020

mpenkov Oct 3, 2020

piskvorky left a comment •

edited

Loading

Replace deprecated smart_open.smart_open function with a thin wrapper #544

Replace deprecated smart_open.smart_open function with a thin wrapper #544

Conversation

mpenkov commented Oct 1, 2020

piskvorky commented Oct 1, 2020 • edited Loading

mpenkov commented Oct 1, 2020 • edited Loading

piskvorky commented Oct 1, 2020 • edited Loading

piskvorky commented Oct 1, 2020 • edited Loading

mpenkov commented Oct 1, 2020

krzemienski commented Oct 1, 2020 • edited Loading

mpenkov commented Oct 2, 2020

gojomo commented Oct 2, 2020

piskvorky commented Oct 2, 2020 • edited Loading

gojomo Oct 3, 2020

Choose a reason for hiding this comment

mpenkov Oct 3, 2020

Choose a reason for hiding this comment

piskvorky left a comment • edited Loading

Choose a reason for hiding this comment

piskvorky commented Oct 1, 2020 •

edited

Loading

mpenkov commented Oct 1, 2020 •

edited

Loading

piskvorky commented Oct 1, 2020 •

edited

Loading

piskvorky commented Oct 1, 2020 •

edited

Loading

krzemienski commented Oct 1, 2020 •

edited

Loading

piskvorky commented Oct 2, 2020 •

edited

Loading

piskvorky left a comment •

edited

Loading