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

Exclude benchmarks from the list of packages found #911

Closed
wants to merge 1 commit into from

Conversation

ttung
Copy link
Contributor

@ttung ttung commented Jan 27, 2020

Description

The benchmarks directory was not meant to be packaged for distribution.

Type of change

  • Bug-fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

References

Fixes #910

How has this been tested?

% python setup.py sdist
% mkdir /tmp/4
% cd /tmp/4
% pip install ~/imaging/napari/dist/napari-0+unknown.tar.gz
% python 
Python 3.7.5 (default, Oct 28 2019, 07:13:20) 
[Clang 11.0.0 (clang-1100.0.33.8)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import benchmarks
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'benchmarks'
>>> ^D
%

Final checklist:

  • My PR is the minimum possible work for the desired functionality
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

It's not meant to be packaged for distribution.

Fixes napari#910
Copy link
Member

@kne42 kne42 left a comment

Choose a reason for hiding this comment

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

thanks!

@sofroniewn sofroniewn added the bug Something isn't working label Jan 27, 2020
@sofroniewn sofroniewn added this to the 0.2.11 milestone Jan 27, 2020
@sofroniewn
Copy link
Contributor

This will close #910. @ttung will the benchmarks will still get included in the pip install? i.e. if i want to run them i can, but they just won't be available as a top level import, or will that code not even get distributed now?

I don't know what the conventions / norms around this are so I'll ultimately defer to others who know more. It makes sense that we shouldn't be adding something that appears under import benchmarks but it might still be nice for people to run the benchmarking suites even if they got napari from a pip install? Or maybe that isn't needed? What does @jni think?

@ttung
Copy link
Contributor Author

ttung commented Jan 27, 2020

I think it's far better to bundle these into napari.benchmarks if you want them distributed.

@sofroniewn
Copy link
Contributor

sofroniewn commented Jan 27, 2020

I think it's far better to bundle these into napari.benchmarks if you want them distributed.

Makes sense - I do see this as quite analogous to #875 too, so we might want to mirror whatever solution we come up with there here - see #912

@ttung
Copy link
Contributor Author

ttung commented Jan 27, 2020

Dumping this one...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

napari should not install "benchmarks" as a toplevel package
3 participants