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

Add recipe for Gensim #3225

Merged
merged 35 commits into from
Sep 4, 2017
Merged

Add recipe for Gensim #3225

merged 35 commits into from
Sep 4, 2017

Conversation

souravsingh
Copy link
Contributor

Adds a new recipe for Gensim.

Ping @tmylk @menshikh-iv

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/gensim) and found it was in an excellent condition.

requires:
- nose
- testfixtures
- unittest
Copy link
Member

Choose a reason for hiding this comment

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

unittest is a builtin

build:
- python
- setuptools
- numpy >=1.3
Copy link
Member

@mariusvniekerk mariusvniekerk Jul 6, 2017

Choose a reason for hiding this comment

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

Since gensim uses cython, i would change this to

  - numpy x.x
  - cython

and change the numpy section in the run area too.

This will build you a version of gensim that is compatible with the numpy ABI for the versions that conda forge supports

@mariusvniekerk
Copy link
Member

So generally we don't run the unit test suite of a library as part of conda forge packaging. Instead we validate that things are importable and correctly linked. Running unit tests is typically done upstream in the project itself.

- setuptools
- numpy x.x
- cython
- scipy >=0.7.0
Copy link
Member

Choose a reason for hiding this comment

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

Please use numpy/scipy versions from develop branch

run:
- python
- numpy x.x
- cython
Copy link
Member

Choose a reason for hiding this comment

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

cython is probably not needed as a runtime dependency


test:
requires:
- nose
Copy link
Member

Choose a reason for hiding this comment

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

Its probably sufficient initially to have

    imports:
        - gensim
        - gensim.foo

for all the standard importable modules

@souravsingh
Copy link
Contributor Author

souravsingh commented Jul 7, 2017

@mariusvniekerk @menshikh-iv The tests on the recipe have passed, but I remember @tmylk saying that the whole test suite must be run in the recipe. Should we go ahead with running the import tests?

@souravsingh
Copy link
Contributor Author

Ping @menshikh-iv

@tmylk
Copy link

tmylk commented Jul 12, 2017

If the gensim suite tests of the assembled conda recipe fail then the package shouldn't be released.

@souravsingh
Copy link
Contributor Author

Thanks @tmylk. I will be replacing the import tests with the full gensim tests.

@souravsingh
Copy link
Contributor Author

@menshikh-iv The test suite is not running due to the unittest2 import. Do we replace all the unittest2 imports with unittest?

- python
- setuptools
- numpy >=1.11.3
- cython

Choose a reason for hiding this comment

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

Why is this needed? I don't think cython is a dependency of gensim.

license_file: COPYING
license_family: LGPL
summary: 'A library for topic modelling and document indexing'
description: |
Copy link

@piskvorky piskvorky Jul 20, 2017

Choose a reason for hiding this comment

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

Please use the official tagline and description of gensim (see https://github.com/RaRe-Technologies/gensim).

Copy link

@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.

Minor doc nitpicks.

license: LGPL 3.0
license_file: COPYING
license_family: LGPL
summary: 'Topic Modelling in Python'

Choose a reason for hiding this comment

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

Tagline = Topic Modelling for Humans.

description: |
Gensim is a Python library for topic modelling, document indexing
and similarity retrieval with large corpora.
Target audience is the natural language processing(NLP)
Copy link

@piskvorky piskvorky Jul 20, 2017

Choose a reason for hiding this comment

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

Missing space before the opening bracket.

license_file: COPYING
license_family: LGPL
summary: 'Topic Modelling in Python'
description: |

Choose a reason for hiding this comment

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

The | is some specific formatter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/gensim) and found some lint.

Here's what I've got...

For recipes/gensim:

  • Selectors are suggested to take a <two spaces>#<one space>[<expression>] form.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/gensim) and found it was in an excellent condition.

@souravsingh
Copy link
Contributor Author

souravsingh commented Aug 27, 2017

@ocefpaf Is it possible to restart Appveyor builds for the recipe? The Python 2.7 build has failed due to a ConnectionError

@ocefpaf
Copy link
Member

ocefpaf commented Aug 27, 2017

Done. But you can also do that by closing and opening the PR.

@souravsingh
Copy link
Contributor Author

souravsingh commented Aug 29, 2017

@piskvorky The recipe is now ready for deployment as a feedstock. The recipe currently supports Linux and Windows 64-bit, since it might create problems for Windows 32-bit if they wish to run the Gensim for a larger corpus.

I have skipped support for OSX for now since Travis times out everytime when we run the nosetests command. I suggest merging the recipe for now

@piskvorky
Copy link

piskvorky commented Aug 29, 2017

Thanks, but I don't have merge rights in this repo. This is for @menshikh-iv -- what is the merging / deployment process here?

@souravsingh
Copy link
Contributor Author

souravsingh commented Aug 29, 2017

@piskvorky First, The PR will be merged by one of the reviewers from conda-forge organization(Filipe handles most of the merging stuff). Once the recipe is merged, The conda-smithy will start creating a feedstock repository for the recipe, something similar to this repository- https://github.com/conda-forge/tensorflow-feedstock and the built tarballs will be uploaded to the conda-forge channel, from where the users can download the tarball directly.

@mariusvniekerk
Copy link
Member

@ocefpaf @ericdill @jakirkham Is this ready to go?

@ocefpaf
Copy link
Member

ocefpaf commented Aug 30, 2017

We are having some trouble in master with the feedstock creation. We'll resume merging PRs as soon as that is solved. (I'll look into the problem soon.)

@menshikh-iv
Copy link
Member

Ignoring MacOS looks suspicious, but we can resolve it later.
LGTM for me, good job @souravsingh👍

@souravsingh
Copy link
Contributor Author

@ocefpaf Is the recipe ready to merge?

@souravsingh
Copy link
Contributor Author

@ocefpaf The Gensim doesn't support Python 3.4. It only supports Python 2.7, 3.5 and 3.6

@ocefpaf
Copy link
Member

ocefpaf commented Sep 1, 2017

@ocefpaf The Gensim doesn't support Python 3.4. It only supports Python 2.7, 3.5 and 3.6

Don't worry about the pre-selector there. Conda-forge does not build Python 3.4. (I added it for reasons 😉)

However, if py34 is not really support you need to add a skip statement.

@menshikh-iv
Copy link
Member

menshikh-iv commented Sep 1, 2017

The Gensim doesn't support Python 3.4. It only supports Python 2.7, 3.5 and 3.6

Incorrect. Gensim supports all versions >= 2.7.

@ocefpaf
Copy link
Member

ocefpaf commented Sep 1, 2017

Incorrect. Gensim supports all versions >= 2.7.

Thanks @menshikh-iv. Good to know!

@souravsingh
Copy link
Contributor Author

souravsingh commented Sep 1, 2017

My bad, I thought Gensim doesn't support Python 3.4 since there is no build matrix for py3.4 in Travis. Thanks for the info @menshikh-iv.

@souravsingh souravsingh closed this Sep 3, 2017
@souravsingh souravsingh reopened this Sep 3, 2017
@ocefpaf ocefpaf merged commit 95eefab into conda-forge:master Sep 4, 2017
@souravsingh souravsingh deleted the add-gensim branch September 4, 2017 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants