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

Break requirements down on per backend basis #64

Merged
merged 8 commits into from Oct 24, 2019
Merged

Break requirements down on per backend basis #64

merged 8 commits into from Oct 24, 2019

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Oct 23, 2019

This PR splits the requirements file further for each backend (e.g. mongo, django and hopefully elasticsearch soon). This should make it easier for people to add more backends in the future, and relates to #63 which introduces additional backend-dependent code (like separate grammars).

  • Each extra set of deps is grabbed automatically in setup.py based on the contents of the requirements/*.txt files.
  • If a backend is not installed, the unit tests will be skipped. As mongo is currently required for the overall package, these are not skipped (though at some point we could just switch to mongomock as the only dep for this, or try to generalise the server for all backends).

@codecov
Copy link

codecov bot commented Oct 23, 2019

Codecov Report

Merging #64 into master will decrease coverage by 0.12%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
- Coverage    78.3%   78.17%   -0.13%     
==========================================
  Files          21       21              
  Lines         719      724       +5     
==========================================
+ Hits          563      566       +3     
- Misses        156      158       +2
Impacted Files Coverage Δ
optimade/filtertransformers/tests/test_django.py 88.23% <71.42%> (-11.77%) ⬇️

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 72e3411...dec3d47. Read the comment docs.

Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

Thanks @ml-evs !

I thought the idea was to have in the setup.py more "lax" requirements (>= or even without version specification), while the requirements.txt files contain fixed dependencies that are used for testing and are guaranteed to work.

This change would remove the two types of requirements - and in that case, I no longer see a reason to have the .txt files; we could just write everything inside the setup.py.

I have to say that I was anyhow not fully convinced by this approach - if one of the newer dependencies does break optimade-python-tools, then users have to know that there are also the more strict requirements.txt files.

How about an intermediate solution, where we don't totally hardcode the version of every dependency but rather hope that they do semantic versioning correctly, i.e. something like
lark-parser>=0.5.6,<0.6
mongomock>=3.16.0,<4
(and only keep this in the setup.py)?

@ml-evs
Copy link
Member Author

ml-evs commented Oct 23, 2019

Hey @ltalirz, I'm happy to revert the changes to setup.py and hardcode the split requirements, I guess I normally work with fixed version reqs but can see the reasons others might not want to.

I think requirements files are still useful so the user can use their package manager of choice (e.g. conda install --file or pip install -r) separate from the setup.py.

@ltalirz
Copy link
Member

ltalirz commented Oct 24, 2019

travis fails - let me know when you want me to review;
the changes look fine to me

@ml-evs
Copy link
Member Author

ml-evs commented Oct 24, 2019

Ah, thought pip could figure out [all] on its own, but guess not. Should be fixed now.

@ml-evs
Copy link
Member Author

ml-evs commented Oct 24, 2019

We could consider switching to Pipfile only for our reqs, as FastAPI have done.

@ltalirz ltalirz self-requested a review October 24, 2019 11:14
ltalirz
ltalirz previously approved these changes Oct 24, 2019
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

thanks @ml-evs , looks good to me

just to understand: what would be needed to be able to remove mongogrant from the default dependencies (which is currently duplicated in the mongo dependency)?

also, would vote to move to pipenv

@ml-evs
Copy link
Member Author

ml-evs commented Oct 24, 2019

thanks @ml-evs , looks good to me

just to understand: what would be needed to be able to remove mongogrant from the default dependencies (which is currently duplicated in the mongo dependency)?

also, would vote to move to pipenv

Nothing! :)

I'm not sure we're using it explicitly in the code, but its a useful tool either way if you're setting up a mongo to use with optimade.

+1 for pipenv, perhaps that's the next (boring) step after we get #63 merged.

@ml-evs ml-evs requested a review from ltalirz October 24, 2019 12:08
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

thanks!

@ml-evs ml-evs merged commit 5e5daf0 into Materials-Consortia:master Oct 24, 2019
@ml-evs ml-evs deleted the ml-evs/req-shuffle branch October 25, 2019 13:33
@ml-evs
Copy link
Member Author

ml-evs commented Oct 25, 2019

Closes #62

@CasperWA CasperWA mentioned this pull request Nov 20, 2019
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.

None yet

2 participants