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

Prepare first upload to PyPI #107

Conversation

HealthyPear
Copy link
Member

This PR prepares the first release that will be uploaded to PyPI as v0.4.0.post1 by updating the setup.py in a way that a pure pip installation will be possible with tests and docs extra requirements.

It is a requirement for PR #105.

It also comes with the following modifications:

  • updated installation instructions for both released and development versions
  • updated .zenodo.json with changes that hopefully will unlock the DOI generation which got stuck with v0.4.0
  • updated README
  • fix .gitignore for dummy files generated by setup.py and related to versioning

@HealthyPear HealthyPear added this to the v0.5.0 milestone Mar 2, 2021
@HealthyPear HealthyPear added this to In progress in Maintenance via automation Mar 2, 2021
@HealthyPear HealthyPear requested a review from kosack March 2, 2021 13:02
@codecov
Copy link

codecov bot commented Mar 2, 2021

Codecov Report

Merging #107 (a82f151) into master (92f3120) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #107      +/-   ##
==========================================
+ Coverage   37.03%   37.06%   +0.03%     
==========================================
  Files          19       20       +1     
  Lines        1801     1802       +1     
==========================================
+ Hits          667      668       +1     
  Misses       1134     1134              
Impacted Files Coverage Δ
protopipe/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92f3120...a82f151. Read the comment docs.

setup.py Outdated
@@ -42,7 +43,7 @@ def readme():
packages=find_packages(),
package_data={"protopipe": ["aux/example_config_files/protopipe/analysis.yaml"]},
include_package_data=True,
install_requires=["ctapipe"],
install_requires=["ctapipe==0.9.1", "jupyterlab", "pyirf", "vitables"],
Copy link
Contributor

@kosack kosack Mar 2, 2021

Choose a reason for hiding this comment

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

is jupyterlab and vitables really required? or only to generate docs? install_requires should only be things that are required to run, not useful things in the user's environment (which would go into environment.yml, or similar). I think the only requirements should be ctapipe and pyirf.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is for a pure pip installation, so after pip install protopipe the user should have all the necessary tools to use protopipe and like this it doesn't really need even conda.
For now "use protopipe" means also benchmarking and checking data, so for me these 2 tools are necessary for the package as a whole.

Copy link
Contributor

@kosack kosack Mar 5, 2021

Choose a reason for hiding this comment

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

I think that is the incorrect way to do it. Pip is not meant for making environments, only for installing required dependencies. For environments you need to use pipenv or poetry, or just provide a requirements.txt file that is the output of pip freeze. In the package itself (in setup.py) you should only list the bare minimum to allow it to work. Otherwise you will run into dependency resolution problems later.

So here you should have only ctapipe and pyirf as dependencies.

For reference: https://packaging.python.org/discussions/install-requires-vs-requirements/:

Whereas install_requires requirements are minimal, requirements files often contain an exhaustive listing of pinned versions for the purpose of achieving repeatable installations of a complete environment.

@HealthyPear HealthyPear requested a review from kosack March 3, 2021 18:00
setup.py Outdated
@@ -42,7 +43,7 @@ def readme():
packages=find_packages(),
package_data={"protopipe": ["aux/example_config_files/protopipe/analysis.yaml"]},
include_package_data=True,
install_requires=["ctapipe"],
install_requires=["ctapipe==0.9.1", "jupyterlab", "pyirf", "vitables"],
Copy link
Contributor

@kosack kosack Mar 5, 2021

Choose a reason for hiding this comment

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

I think that is the incorrect way to do it. Pip is not meant for making environments, only for installing required dependencies. For environments you need to use pipenv or poetry, or just provide a requirements.txt file that is the output of pip freeze. In the package itself (in setup.py) you should only list the bare minimum to allow it to work. Otherwise you will run into dependency resolution problems later.

So here you should have only ctapipe and pyirf as dependencies.

For reference: https://packaging.python.org/discussions/install-requires-vs-requirements/:

Whereas install_requires requirements are minimal, requirements files often contain an exhaustive listing of pinned versions for the purpose of achieving repeatable installations of a complete environment.

Maintenance automation moved this from In progress to Review in progress Mar 5, 2021
@HealthyPear HealthyPear requested a review from kosack March 5, 2021 17:02
Maintenance automation moved this from Review in progress to Reviewer approved Mar 5, 2021
@HealthyPear HealthyPear merged commit 36d6880 into cta-observatory:master Mar 5, 2021
Maintenance automation moved this from Reviewer approved to Done Mar 5, 2021
@HealthyPear HealthyPear deleted the maintenance-prepare_1st_pypi_upload branch March 5, 2021 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Maintenance
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants