-
Notifications
You must be signed in to change notification settings - Fork 22
1333: sync requirements.txt and pyproject.toml #1373
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
base: main
Are you sure you want to change the base?
1333: sync requirements.txt and pyproject.toml #1373
Conversation
I have validated that on python3.12 |
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 think it would be a good idea to separate our requirements into runtime dependancies in the requirement.txt and have a requirement-dev.txt with the development tools/linters/etc that also references '-r requirements.txt' in it to limit package size of our executable. That way we can pull in the runtime dependencies into the build while excluding all the other packages we dont need. We would also maybe need to update documents to reference requirement-dev.txt when installing dependencies in our readme. What do you think about this proposal?
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.
The PR pushes a change that synchronizes the pyproject.toml file with the requirements.txt file to keep both coherent. The validation was done by installing the CDISC rules engine project locally using pip. The install was successful and had same version as in requirements.txt.
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.
this only needs the requirements-dev.txt if we reference requirements.txt in the dev 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.
sorry not getting your point... we basically need to install dev dependencies here to make pytest work
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.
this is in reference to what i wrote below:
"I would add a line -r requirement.txt to this file at the top so users can install all requirements for dev and runtime with just pip install requirement-dev.txt"
If installing dev requirements has a reference to requirement.txt, then we only need pip install -r requirements-dev.txt
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.
gotcha, just updated
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 -- just get rid of this one readme line and we can merge
|
||
- Install the requirements. | ||
|
||
`python -m pip install -r requirements.txt` # From the root directory |
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.
eliminate this line-- just need to install python -m pip install -r requirements-dev.txt
No description provided.