-
Notifications
You must be signed in to change notification settings - Fork 0
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
Pin dependencies #61
Pin dependencies #61
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just a small comment about the versioning.
Should we add the information for generating the requirements.txt
to the README?
@@ -0,0 +1,15 @@ | |||
dune-client | |||
moralis | |||
pandas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment this actually removes the version pinning for these packages:
requests==2.31.0
web3==7.0.0
pandas==2.2.1
SQLAlchemy==2.0.28
psycopg2==2.9.9
python-dotenv==1.0.0
If we want to keep them pinned we should add that here. For other packages I think it might be worth pinning major versions as well just because it can't hurt. So for example we could set something like dune-client>=1.7.5,<2.0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are any of those restrictions necessary at the moment?
I would have expected that one uses as few restrictions as possible in requirements.in
. Any recompiling will require some form of testing of the new dependencies anyways. At that stage one can add restrictions or update the code.
Restricting on major versions sounds OK. I would probably only do it after we see an issue with some version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bram-vdberg I actually see all these packages you listed fully pinned down in this PR, which i guess is the expected behavior as they are listed in the requirements.in file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@harisang I meant that they're pinned in the requirements.txt file, but not in the requirements.in file. This means that recompiling the requirements.in could upgrade these packages. Another way to do this is to pin selected packages in the requirements.in, that means that we can recompile and upgrade downstream dependencies without upgrading these packages.
I think Felix is right that we don't need to do that unless necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me!
This pins all dependencies using
pip-tools
. Depedencies are changed inrequirements.in
and compiled intorequirements.txt
viaUpdating requirements requires
pip-tools
which can be installed globally viapipx install pip-tools
orpip install pip-tools
.I do not know how to test these changes. The tests in the repo are failing without this change and most functionality is not tests at all.