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

Update handling of installation requirements #90

Merged
merged 4 commits into from
Aug 23, 2021

Conversation

cthoyt
Copy link
Member

@cthoyt cthoyt commented Jul 8, 2021

This PR updates the way requirements are installed. Notably, it removes the requirements.txt, Pipfile, and Pipfile.lock since those are typically only used for web applications and one-off scripts. There's the little caveat where this means some installation dependencies can get mixed up, but Python is provably bad at this, and it's a crapshoot anyway (especially if you expect someone to install your software).

The requirements for docs and tests get put in their own extras inside the setup.cfg. All installation is changed to use pip. Tox allows you to specify which extras you want (should be obvious from the diff)

@matentzn
Copy link
Collaborator

matentzn commented Jul 9, 2021

Thank you so much @cthoyt again! :) Two things:

  1. How do pypi releases work with this infrastructure? Is this still ok:
pypi: test
	echo "Uploading to pypi. Make sure you have twine installed.."
	python setup.py sdist
	twine upload dist/*
  1. Do you know why the QC is not triggered for PRs from forks?

@cthoyt
Copy link
Member Author

cthoyt commented Jul 9, 2021

  1. Yes it works exactly the same way
  2. PR to fix this at Make QC run on pull requests #93

@matentzn matentzn requested a review from cmungall July 9, 2021 12:56
@matentzn
Copy link
Collaborator

matentzn commented Jul 9, 2021

I am happy to trust you on this @cthoyt - nothing here can be reviewed by mere. Lets give @cmungall a chance to take look when he can.

@cthoyt
Copy link
Member Author

cthoyt commented Jul 9, 2021

Btw @matentzn I use tox to take care of the installation stuff instead of make, since it automatically creates a virtualenvironment with the dependencies you need to build and upload to PyPI. See my example at
https://github.com/biomappings/biomappings/blob/7dff7f12d6c99683e8a46718e01d3cf285dc8551/tox.ini#L66-L81

@matentzn matentzn marked this pull request as draft July 20, 2021 16:29
@matentzn matentzn self-assigned this Jul 23, 2021
setup.cfg Outdated
networkx
pandasql
sparqlwrapper
jsonasobj~=1.2.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this required? isn't this a transitive dependency?

I feel this may have been put in as a patch at some point. I think if removed everything will work fine

Copy link
Member Author

Choose a reason for hiding this comment

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

I would say including transitive dependencies in setup.cfg is explicitly bad. I didn't carefully read all requirements list but rather copied this over from the requirements.txt file, so I would be happy to remove ones that aren't explicitly imported in the package itself. However, any dependency that's explicitly imported in a package should explicitly list it as a dependency. I hope the difference makes sense, let me know if not and I will elaborate

@cmungall
Copy link
Collaborator

sorry I didn't realize I was the blocker.

Let's do it! @hrshdhgd let's prioritize! cc @sierra-moxon to coordinate with our cookiecutter template

one thing I don't understand is do we remove all version info from dependencies altogether? I agree this is a nightmare, and I don't like pinning to specific versions since it inhibits reuse.

But there are some versions of some packages we know just won't work. isn't nx finicky? I like to at least be able to specify a major version requirements

@cthoyt
Copy link
Member Author

cthoyt commented Aug 21, 2021

I think doing minimum versions is good if you're taking advantage of a feature that got introduced at a certain point. For some packages you might also want to specify a maximum version (e.g., I've done it for RDFlib) but this is typically a bad move because it means you don't support the newest code.

Overall I think it's still fine to play it fast and loose w.r.t. version numbers until disaster occurs :)

@matentzn matentzn changed the base branch from master to installation August 23, 2021 14:13
@matentzn matentzn closed this Aug 23, 2021
@matentzn matentzn reopened this Aug 23, 2021
@matentzn
Copy link
Collaborator

Closing in favour of #113

@matentzn matentzn closed this Aug 23, 2021
@cthoyt
Copy link
Member Author

cthoyt commented Aug 23, 2021

@matentzn why? Couldn't you have just made updates to this branch? Plus what you did makes it hard to track/attribute the updates

@matentzn
Copy link
Collaborator

Ok, can you move it then? Since we will work on the branch, I want it to be on this repo - but due to conflicts I couldn't move it.. If you want, just reopen your pull request again but not to master, and with conflicts resolved.

@cthoyt
Copy link
Member Author

cthoyt commented Aug 23, 2021

@matentzn btw there's a button on the right "Allow edits by maintainers" that I didn't check. Most repos have it checked by default - you can change this in the settings for the repo so new PRs will automatically be able to be edited by the maintainers

@matentzn
Copy link
Collaborator

Did you allow edits now?

@matentzn matentzn reopened this Aug 23, 2021
@matentzn matentzn marked this pull request as ready for review August 23, 2021 15:08
@matentzn matentzn merged commit cffd4bc into mapping-commons:installation Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants