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

Removed fuzzytm dependency. Fixes #3423. #3428

Closed
wants to merge 1 commit into from

Conversation

ERijck
Copy link
Contributor

@ERijck ERijck commented Jan 11, 2023

Fixes #3423. Supersedes #3424.

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.

Thanks @ERijck . You're becoming a git pro :-)

Can you also answer #3424 (comment) please – what's the deal with the dependencies now? For example where does pyfume come from? Is it clearly documented (in a tutorial / import error message / module docstring) how to obtain that dependency? Is there a specific version of pyfume needed?

(there might be other dependencies; I only noticed pyfume in my cursory look)

@@ -344,7 +344,7 @@ def run(self):
NUMPY_STR,
'scipy >= 1.7.0',
'smart_open >= 1.8.1',
'FuzzyTM >= 0.4.0'

Copy link
Owner

Choose a reason for hiding this comment

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

No need for the blank line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, thanks for your help and patience @piskvorky! I have replied to your comment in the original thread.

@osma
Copy link

osma commented Jan 12, 2023

Forgive my ignorance, but wouldn't it be easiest to just add pyfume as a dependency in setup.py? Or is there a reason why you want to make it an optional extra?

@piskvorky
Copy link
Owner

piskvorky commented Jan 12, 2023

We definitely don't want an additional core dependency, just because of one new module. pyfume is not quite there at the level of numpy, in terms of its footprint on the Gensim architecture.

@piskvorky
Copy link
Owner

@ERijck can you finish this up?

The current (latest) version of released Gensim is not good, I'd like to release the bugfix ASAP. Thanks!

@piskvorky piskvorky added this to the Next release milestone Jan 16, 2023
@ERijck
Copy link
Contributor Author

ERijck commented Jan 16, 2023

@piskvorky do you mean with 'this' that I add an ImportError if pyfume is not installed and then show to install it?

@piskvorky
Copy link
Owner

No, I meant my review of this PR (above).

But the import stuff we discussed in #3424 (comment) is also relevant.

@ERijck
Copy link
Contributor Author

ERijck commented Jan 16, 2023

In your review you mention dependencies. By adding an ImportError, I thought I covered that issue. Do you agree?
Also, I will remove the blank line.

@piskvorky
Copy link
Owner

What do you mean by "by adding an ImportError"? I don't see that in this PR.

@ERijck
Copy link
Contributor Author

ERijck commented Jan 17, 2023

In this PR you mention adding an 'import error message'. That is what I meant.

@piskvorky
Copy link
Owner

piskvorky commented Jan 17, 2023

Right – but I do not see it added in this PR.

To be clear, what I mean is: what happens when someone imports this module without having its necessary dependencies (pyfume, maybe others)? Do they get a meaningful, actionable error message?

If it's just a question of pyfume, and pyfume is on standard PyPI, it should be trivial: FlsaModel requires pyfume version ????, but pyfume could not be imported. Please install pyfume (e.g. with ``pip install pyfume==?????``) and try again.

But if the dependency/dependencies are not on PyPI, the instructions could be more involved.

@ERijck ERijck closed this Jan 17, 2023
@ERijck ERijck deleted the no-fuzzytm-dependency branch January 17, 2023 11:32
@ERijck ERijck restored the no-fuzzytm-dependency branch January 17, 2023 11:44
This was referenced Jan 17, 2023
@ERijck ERijck deleted the no-fuzzytm-dependency branch January 23, 2023 15:51
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.

Unnecessary dependency on FuzzyTM pulls in many libraries
3 participants