-
Notifications
You must be signed in to change notification settings - Fork 68
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
SYNPY-1285: Create pipfile #984
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.
🤖 Nice, just a comment
Pipfile
Outdated
cryptography = "3.3.2" | ||
deprecated = "1.2.14" | ||
keyring = "23.4.1" | ||
pandas = "2.0.3" |
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.
There's some packages we don't require for users:
Lines 87 to 94 in 2747b12
pandas = | |
pandas>=1.5,<2.1 | |
pysftp = | |
pysftp>=0.2.8,<0.3 | |
boto3 = | |
boto3>=1.7.0,<2.0 |
We can certainly lock the version down or use the ~=
function to be a bit more permissive. Thoughts?
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.
I'll move those packages over.
For versioning IMO I didn't like giving ranges that much when dealing with lock files - Since regenerating the lock file is going to update many dependencies that might not have been intentionally changed. Though I am ok with whichever direction.
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.
Could we follow something like this where the pipfile pulls from a setup.cfg and/or pyproject.toml: https://github.com/Sage-Bionetworks-Workflows/py-orca/blob/e3ddde039e2776c6b8f053f7c502fc05dab6f9cd/Pipfile#L6-L13. One concern I have is that the python community is moving away from setup.cfg as stated in pep-621: https://peps.python.org/pep-0621/.
I agree we shouldn't do ranges, but lets do something with this (~=
): https://peps.python.org/pep-0440/#compatible-release
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.
Great idea. I will explore this and add a new commit with the changes.
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.
I modified the pipfile to pull the dependencies from the setup.cfg
like py-orca
is doing - Is this what you had in mind?
fc0f18d
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.
🔥 LGTM!
Problem:
Setting up the environment to start developing for the synapse python client was a mix of automated and manual steps. In order to stream line jumping into the project for future developers and to maintain dependencies for further development we want to leverge
pipenv
and virtual environments.Solution:
I created a pipfile based off the versioning in the
setup.cfg
.Testing:
pipenv install
and verified I could use the CLI and any commands I wanted to use when importing thesynapseclient
into my python file.pipenv install --dev
and verified that I could now runpytest
- I ran the tests for bothunit
andintegration
tests on my local machine.