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 Biogeme #26374

Merged
merged 94 commits into from
Aug 31, 2024
Merged

Add Biogeme #26374

merged 94 commits into from
Aug 31, 2024

Conversation

FGarridoV
Copy link
Contributor

@FGarridoV FGarridoV commented May 16, 2024

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

@conda-forge-webservices
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/biogeme, recipes/biogeme-optimization, recipes/cythonbiogeme) and found it was in an excellent condition.

@conda-forge-webservices
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/biogeme) and found it was in an excellent condition.

- scipy >=1.7.3
- tqdm >=4.64.1
- tomlkit >=0.11.5
- cythonbiogeme ==1.0.2
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not present on conda forge, afaik. You should include a recipe for it in this PR also.

home: http://biogeme.epfl.ch
summary: Estimation and application of discrete choice models
license: MIT
license_family: MIT
Copy link
Contributor

Choose a reason for hiding this comment

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

This license does not seem to be MIT: https://github.com/michelbierlaire/biogeme/blob/master/LICENSE

It seems to be a modification of the MIT license. I'm not sure we can actually distribute this package, given the license.

Copy link
Contributor

Choose a reason for hiding this comment

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

MIT license:

Copyright (c) <year> <copyright holders>

Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.

License found in biogeme:

BIOGEME is distributed free of charge. We ask each user to register to
Biogeme's users group, and to mention explicitly the use of the
package when publishing results, using the following reference:

For BisonBiogeme: Bierlaire, M. (2003). BIOGEME: A free package for
the estimation of discrete choice models , Proceedings of the 3rd
Swiss Transportation Research Conference, Ascona, Switzerland.

For PythonBiogeme: Bierlaire, M. (2016) PythonBiogeme: a short
introduction. Report TRANSP-OR 160706 ,Series on Biogeme. Transport
and Mobility Laboratory, School of Architecture, Civil and
Environmental Engineering, Ecole Polytechnique Fédérale de Lausanne,
Switzerland.

Disclaimer: This software is provided free of charge and "AS IS"
WITHOUT ANY WARRANTY of any kind. The implied warranties of
merchantability, fitness for a particular purpose and non-infringment
are expressly disclaimed. In no event will the author (Michel
Bierlaire) or his employer (EPFL) be liable to any party for any
direct, indirect, special or other consequential damages for any use
of the code including, without limitation, any lost profits, business
interruption, loss of programs or other data on your information
handling system or otherwise, even if we are expressly advised of the
possibility of such damages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove license and license_family since this is not the MIT license.

Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be license: Custom, same as I did in cythonbiogeme.

@conda-forge-webservices
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/biogeme, recipes/biogeme-optimization, recipes/cythonbiogeme) and found some lint.

Here's what I've got...

For recipes/biogeme:

  • There are too few lines. There should be one empty line at the end of the file.

For recipes/biogeme-optimization:

  • There are too few lines. There should be one empty line at the end of the file.

For recipes/cythonbiogeme:

  • There are too few lines. There should be one empty line at the end of the file.

@conda-forge-webservices
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/biogeme, recipes/biogeme-optimization, recipes/cythonbiogeme) and found some lint.

Here's what I've got...

For recipes/biogeme:

  • There are too few lines. There should be one empty line at the end of the file.

For recipes/cythonbiogeme:

  • There are too few lines. There should be one empty line at the end of the file.

@conda-forge-webservices
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/biogeme, recipes/biogeme-optimization, recipes/cythonbiogeme) and found it was in an excellent condition.

sha256: f139342944c849b47ce5aaa548e68aa179c195de93ab76be78353f4309684d0e

build:
number: 3
Copy link
Contributor

Choose a reason for hiding this comment

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

Build number should start at 0.

summary: Various optimization algorithms for teaching and research
license: MIT
license_family: MIT
license_file: LICENSE.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the error from the linux build:

ValueError: License file given in about/license_file (/home/conda/staged-recipes-copy/recipes/biogeme-optimization/LICENSE.txt) does not exist in source root dir or in recipe root dir (with meta.yaml)

Copy link
Contributor

@moorepants moorepants May 21, 2024

Choose a reason for hiding this comment

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

There is no license file in the source tarball for this package. In fact, there is no evidence of a license in the repo at all: https://github.com/michelbierlaire/optimization

If there is no license at all, I don't think we can add this to conda forge because no permission to redistribute is given. Maybe you can open an issue on the author's repository and ask if he will grant permission or add an OSS license to the repository.

@moorepants
Copy link
Contributor

cythonbiogeme osx failure:

  In file included from /Users/runner/Miniforge3/conda-bld/cythonbiogeme_1716277750463/_build_env/bin/../include/c++/v1/__mbstate_t.h:45:
  /Applications/Xcode_14.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/usr/include/sys/_types/_mbstate_t.h:31:9: error: unknown type name '__darwin_mbstate_t'
  typedef __darwin_mbstate_t mbstate_t;
          ^
  1 error generated.
  error: command '/Users/runner/Miniforge3/conda-bld/cythonbiogeme_1716277750463/_build_env/bin/clang++' failed with exit code 1
  error: subprocess-exited-with-error
  
  × python setup.py bdist_wheel did not run successfully.
  │ exit code: 1
  ╰─> See above for output.

summary: C++ part of the Biogeme package
license: MIT
license_family: MIT
license_file: LICENSE.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

Windows build error:

ValueError: License file given in about/license_file (D:\a\1\s\recipes\cythonbiogeme\LICENSE.txt) does not exist in source root dir or in recipe root dir (with meta.yaml)

summary: C++ part of the Biogeme package
license: MIT
license_family: MIT
license_file: LICENSE.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
license_file: LICENSE.txt
license_file: LICENSE


build:
number: 3
script: /bin/rm -f pyproject.toml && {{ PYTHON }} -m pip install . -vv # [unix]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
script: /bin/rm -f pyproject.toml && {{ PYTHON }} -m pip install . -vv # [unix]
script: {{ PYTHON }} -m pip install . -vv

Should be able to keep pyproject.toml and this should run on windows too (at least to start to see if we need a special script.

build:
number: 3
script: /bin/rm -f pyproject.toml && {{ PYTHON }} -m pip install . -vv # [unix]
skip: true # [python_impl == 'pypy']
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line, we'll see if it runs on pypy later.

- pip
run:
- python
- cython >=0.29.16
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't need this a run dependency, remove.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind I see it in the setup.cfg:

install_requires =
cython >= 0.29.16
pandas >= 1.3.5

Does the package compile things at run time?

@moorepants
Copy link
Contributor

cythonbiogeme builds for me locally on linux with the suggested changes I made above.

@conda-forge-webservices
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/biogeme/meta.yaml, recipes/biogeme-optimization/meta.yaml) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipes/biogeme/meta.yaml:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

For recipes/biogeme-optimization/meta.yaml:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

- python-levenshtein >=0.25.1
- fuzzywuzzy >=0.18.0
- cythonbiogeme ==1.0.4
- biogeme_optimization ==0.0.10
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- biogeme_optimization ==0.0.10
- biogeme-optimization ==0.0.10

@FGarridoV
Copy link
Contributor Author

@moorepants Finally!! It seems that there no issues now!

@sandervancranenburgh
Copy link
Contributor

Wonderful!!

@FGarridoV
Copy link
Contributor Author

FGarridoV commented Aug 22, 2024

@conda-forge/help-python-c This is ready for review.

The license is a custom license that does not have an SPDX identifier. The author gave consent to package for conda forge in #27157. I used "Custom" as the license name and the license is packaged.

We are hoping to get this included in our annual software deployment at my university before classes start Sept 1, so if this can be merged soon, that would be much appreciated. I can iterate in the feedstock if there are any other issues.

@mwcraig I would like your assistance as you were involved in cythonbiogeme (a dependency of biogeme) Thank you!

@conda-forge-webservices
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/biogeme/meta.yaml, recipes/biogeme-optimization/meta.yaml) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipes/biogeme/meta.yaml:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

For recipes/biogeme-optimization/meta.yaml:

  • No valid build backend found for Python recipe for package biogeme-optimization using pip. Python recipes using pip need to explicitly specify a build backend in the host section. If your recipe has built with only pip in the host section in the past, you likely should add setuptools to the host section of your recipe.
  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

@sandervancranenburgh
Copy link
Contributor

@moorepants
So, now we just need to wait for the conda-forge team to pull biogeme and biogeme-optimization. And, then we can test it?

@moorepants
Copy link
Contributor

So, now we just need to wait for the conda-forge team to pull biogeme and biogeme-optimization. And, then we can test it?

Yes.

about:
home: http://biogeme.epfl.ch
summary: Various optimization algorithms for teaching and research
license: Custom
Copy link
Member

Choose a reason for hiding this comment

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

Please make it SPDX

Suggested change
license: Custom
license: LicenseRef-BIOGEME

Copy link
Contributor

Choose a reason for hiding this comment

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

The license has been updated, in line with the naming convention. Pls review

recipes/biogeme-optimization/meta.yaml Show resolved Hide resolved
@conda-forge-webservices
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/biogeme/meta.yaml, recipes/biogeme-optimization/meta.yaml) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipes/biogeme/meta.yaml:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

For recipes/biogeme-optimization/meta.yaml:

  • No valid build backend found for Python recipe for package biogeme-optimization using pip. Python recipes using pip need to explicitly specify a build backend in the host section. If your recipe has built with only pip in the host section in the past, you likely should add setuptools to the host section of your recipe.

@conda-forge-webservices
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/biogeme/meta.yaml, recipes/biogeme-optimization/meta.yaml) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipes/biogeme-optimization/meta.yaml:

  • No valid build backend found for Python recipe for package biogeme-optimization using pip. Python recipes using pip need to explicitly specify a build backend in the host section. If your recipe has built with only pip in the host section in the past, you likely should add setuptools to the host section of your recipe.

@conda-forge-webservices
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/biogeme/meta.yaml, recipes/biogeme-optimization/meta.yaml) and found it was in an excellent condition.

@sandervancranenburgh
Copy link
Contributor

@xhochy it seems all remaining issues have been addressed. Please review to code to merge the PR.

Copy link
Contributor

@mwcraig mwcraig left a comment

Choose a reason for hiding this comment

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

This is almost ready to go -- just a couple version restrictions to fix, and ideally you would add the dev_url to each recipe.

The versions are important to get right to make sure the user doesn't end up with an unsupported combination (e.g. numpy 1.26)

- pip
run:
- python >=3.7
- numpy
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- numpy
- numpy >=2

There is a version restriction in the biogeme-optimization dependencies: https://github.com/michelbierlaire/optimization/blob/main/pyproject.toml#L28

Copy link
Contributor

@moorepants moorepants Aug 30, 2024

Choose a reason for hiding this comment

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

The author of biogeme compiled cythonbiogeme against numpy 2, so it should be fine with numpy 1. We really need to be able to install this with numpy 1 and that is why I recommended not following upstream. I've opened an issue upstream to see if the author will correct this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I will help manage this in the feedstock repo and if we really have to add a restriction to numpy 2 we will, but there should be no issues with this and if someone reports something on the feedstock I can give my word to help resolve it. Can that possibly be sufficient? I maintain many feedstocks and haven't had any past issues with similar decisions when upstream authors are a bit to heavy with the pins.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, ok, thanks for the explanation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also @moorepants should I add you as a maintainer on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, and you have no objection to adding the dev_url, I assume? If not, then I'll wait a couple hours for @FGarridoV to chime, then push the necessary changes and merge. That way you can both start testing and tuning as needed. I know you have an imminent deadline.

Copy link
Contributor

Choose a reason for hiding this comment

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

thx!

home: http://biogeme.epfl.ch
summary: Various optimization algorithms for teaching and research
license: LicenseRef-BIOGEME-OPTIMIZATION
license_file: LICENSE
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
license_file: LICENSE
license_file: LICENSE
dev_url: https://github.com/michelbierlaire/optimization

This one took a little while to find 😀, so listing it explicitly would be helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have added this dev_url

- cythonbiogeme ==1.0.4
- biogeme-optimization ==0.0.10
- Jinja2 >=3.1.4
- ipython
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- ipython
- ipython >=8.25.0

Please include the version restriction

Copy link
Contributor

Choose a reason for hiding this comment

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

ipython isn't even really a dependency, it is never imported in the modules. I think this is also a mistake from the upstream author.

Copy link
Contributor

Choose a reason for hiding this comment

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

This one seems harmless to include now, and it can always be removed later.

home: http://biogeme.epfl.ch
summary: Estimation and application of discrete choice models
license: LicenseRef-BIOGEME
license_file: LICENSE
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
license_file: LICENSE
license_file: LICENSE
dev_url: https://github.com/michelbierlaire/biogeme

The dev URL is really handy for checking for dependency changes when the conda-forge bot opens pull requests for new versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have added this dev_url

@mwcraig
Copy link
Contributor

mwcraig commented Aug 31, 2024

I wasn't able to add @moorepants as a maintainer, so I'll merge this and he can be added in the feedstock repos. Thanks everyone for wrapping this one up.

@mwcraig mwcraig merged commit 2df8284 into conda-forge:main Aug 31, 2024
5 checks passed
@moorepants
Copy link
Contributor

Thanks!

@EwoutH
Copy link

EwoutH commented Aug 31, 2024

Exciting to see this merged! And just in time for the new academic year.

Thanks for everyone’s efforts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants