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

Remove wrappers and other cruft #2972

Merged
merged 17 commits into from
Jan 17, 2021
Merged

Remove wrappers and other cruft #2972

merged 17 commits into from
Jan 17, 2021

Conversation

mpenkov
Copy link
Collaborator

@mpenkov mpenkov commented Oct 3, 2020

Removed gensim/models/wrappers, associated tests and documentation entirely.

Removed gensim/sklearn_api entirely.

Removed simserver.rst - seems like something really ancient (e.g. https://radimrehurek.com/gensim/simserver.html).

@piskvorky Some questions:

  • Is there anything else worth removing?
  • What should we do about the coherence model tests? They are useless without the wrappers.

@piskvorky
Copy link
Owner

Thanks! Can you add a summary into the Migration wiki too please? So I don't forget to describe it.

@piskvorky piskvorky added this to the 4.0.0 milestone Oct 28, 2020
@piskvorky
Copy link
Owner

piskvorky commented Oct 28, 2020

Aaaah, here it is. This PR wasn't labeled as "Milestone 4.0" nor linked to its issue #2852, so I couldn't find it. But I knew there was more :)

Is there anything else worth removing?

Yes, for sure. My dilemma is that I'm not sure whether I dislike some modules simply because I never used them (~NIH), or because they're objectively useless / poor.

Although my intuition leans heavily toward the latter.

Problematic modules by their mailing list activity:

  • Group 1: Desirable but weak execution. Top candidates that people report issues with (=try to use and fail) are HDP, summarization, dtm, pattern and the mallet wrapper: So either fix / implement properly (unrealistic for us unfortunately) or remove those.
  • Group 2: Unclear. More obscure wrappers with occasional reports are tensorflow/keras, sklearn, wordrank. Remove?
  • Group 3: Never-heard-from-users. Pivoted normalization, poincare, vowpalwabbit, simserver, ?more? (not top of my mind precisely because never discussed). Remove.

What should we do about the coherence model tests? They are useless without the wrappers.

I'm not sure, would have to take a closer look.

CC @gojomo any candidates from your POV?

@piskvorky piskvorky added the breaks backward-compatibility Change breaks backward compatibility label Oct 28, 2020
This file relies on wrappers that aren't there anymore.
@mpenkov
Copy link
Collaborator Author

mpenkov commented Oct 28, 2020

From my point of view, the problem is that I don't have a full understanding of what needs to go and what needs to stay. Once that gets decided, I can execute relatively quickly.

Of course, making the big decision is the hardest part of this task.

@piskvorky
Copy link
Owner

OK. Not blocking for 4.0.0beta.

@piskvorky piskvorky mentioned this pull request Jan 11, 2021
@mpenkov mpenkov merged commit a21d9cc into develop Jan 17, 2021
@mpenkov mpenkov deleted the rm_wrappers branch January 17, 2021 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks backward-compatibility Change breaks backward compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants