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

Reduce gensim surface area #2852

Closed
7 tasks done
mpenkov opened this issue Jun 10, 2020 · 14 comments
Closed
7 tasks done

Reduce gensim surface area #2852

mpenkov opened this issue Jun 10, 2020 · 14 comments
Assignees
Labels
breaks backward-compatibility Change breaks backward compatibility housekeeping internal tasks and processes impact HIGH Show-stopper for affected users reach LOW Affects only niche use-case users
Milestone

Comments

@mpenkov
Copy link
Collaborator

mpenkov commented Jun 10, 2020

Are there subpackages/submodules that we're not maintaining anymore, and could remove?

  • summarization
  • HDP
  • wordrank
  • dependency on pattern
  • various wrappers (incl. sklearn) and others.
  • simserver documentation
  • viz

The goal is to reduce the maintenance burden of the project.

@piskvorky
Copy link
Owner

piskvorky commented Oct 16, 2020

@mpenkov any other modules to be removed?

I'd like to finalize this even before the pre-release, so the documentation is up-to-date.

Plus document what was removed + why, either here or in the migration notes. Thanks.

@piskvorky
Copy link
Owner

PoincareModel is another such candidate. Not touched by the recent refactoring and upgrades, it's probably out of date and useless.

@mpenkov
Copy link
Collaborator Author

mpenkov commented Feb 27, 2021

@piskvorky I had a look at Poincare model. Yes, it's old and hasn't changed much recently, but it's fairly well tested (I eyeballed the tests and they make sense to me, and they all pass). Do you really want to remove it?

@piskvorky
Copy link
Owner

No, if it's good, we can keep it.

I don't remember many user complaints / bug reports about the Poincare model, although that may be simply because nobody uses it.

@mpenkov
Copy link
Collaborator Author

mpenkov commented Feb 27, 2021

@piskvorky Perhaps we can remove just the visualization subpackage? That doesn't look like it belongs in a library like gensim

https://github.com/RaRe-Technologies/gensim/tree/develop/gensim/viz

@piskvorky
Copy link
Owner

piskvorky commented Feb 27, 2021

viz seems to contain a single module, with two functions related to Poincare embeddings. So if we keep Poincare, maybe move those two functions to its module? (without introducing new dependencies, so import-on-demand for external libs)

Or move those functions to the particular documentation / tutorial that needs viz? Or remove them altogether? I really don't know the context, sorry. But it's a silly subpackage for sure.

@mpenkov
Copy link
Collaborator Author

mpenkov commented Feb 27, 2021

The viz stuff is untested and requires plotly as a dependency (which is in setup.py, meaning all installs include it). My suggestion is to remove viz altogether, and remove the dependency. Visualization is too use case-specific, everyone will want to do it differently anyway.

@piskvorky
Copy link
Owner

Yes, but why is it there? Used by some tutorial / notebook / guide?

@mpenkov
Copy link
Collaborator Author

mpenkov commented Feb 27, 2021

As far as I can tell, it isn't used or mentioned anywhere outside of the automatically generated documentation.

@piskvorky
Copy link
Owner

piskvorky commented Feb 27, 2021

Hm, double weird. I guess we can just remove it then.

requires plotly as a dependency (which is in setup.py, meaning all installs include it)

You mean all Gensim installs require plotly, because of viz? I hope not.

@mpenkov
Copy link
Collaborator Author

mpenkov commented Feb 27, 2021

No, on second examination, only docs builds require it, but nevertheless, it isn't something we need to keep.

@mpenkov
Copy link
Collaborator Author

mpenkov commented Mar 9, 2021

OK, I think I've addressed all the candidates we discussed the last time we talked about this. Is there anything else we need to remove?

@mpenkov
Copy link
Collaborator Author

mpenkov commented Mar 9, 2021

Misha TODO

@mpenkov
Copy link
Collaborator Author

mpenkov commented Mar 16, 2021

Related: #3077

@mpenkov mpenkov closed this as completed Mar 19, 2021
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 housekeeping internal tasks and processes impact HIGH Show-stopper for affected users reach LOW Affects only niche use-case users
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants