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.

Sign up for GitHub

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

Jump to bottom

Reduce requirements #594

Merged
merged 60 commits into from
Aug 24, 2023
Merged

Reduce requirements #594

merged 60 commits into from
Aug 24, 2023

Conversation

jesper-friis
Copy link
Collaborator

@jesper-friis jesper-friis commented Aug 14, 2023 •
edited
Loading

Description

Closes #517

Added new requirements file requirements_full.txt with all optional requirements.

Getting rust, cargo and poetry properly installed in the docker images for building the wheels is followed up in issue #601.

Follow up with issue #602 - Avoid skipping tests when build Python wheels.

Remaining things to be done:

  • Fix failing tests
  • Make it possible to install dlite with pip install DLite-Python[full] to include all optional dependencies.
  • Document how to install DLite with all dependencies included.
  • Ensure that tests run if yaml is missing (avoid false CI failures on this PR before we get everything correctly installed in the docker containers used for building the wheels. This should not be used to skip yaml-dependent tests when testing the wheels).
  • Agree on whether to accept this PR or the solution suggested in issue Create a dlite-python-minimal pypi package #603.

Type of change

  • Bug fix & code cleanup
  • New feature
  • Documentation update
  • Test update

Checklist for the reviewer

This checklist should be used as a help for the reviewer.

  • Is the change limited to one issue?
  • Does this PR close the issue?
  • Is the code easy to read and understand?
  • Do all new feature have an accompanying new test?
  • Has the documentation been updated as necessary?

@jesper-friis jesper-friis linked an issue Aug 14, 2023 that may be closed by this pull request
Clean up requirements #517
Closed
@jesper-friis jesper-friis changed the title Clean up requirements WIP: Clean up requirements Aug 14, 2023
@jesper-friis jesper-friis marked this pull request as draft August 14, 2023 14:18
jesper-friis and others added 24 commits August 14, 2023 17:58
@jesper-friis
Updated README
34c06ba
@jesper-friis
Added missing requirements_full.txt
f8daee8
@jesper-friis
Merge branch 'master' into 517-clean-up-requirements
0a37a1d
@jesper-friis
Require Pydantic v1 until PR #592 has been merged
bbce329
@jesper-friis
Merge branch 'master' into 517-clean-up-requirements
a1d9ca6
@jesper-friis
Added typing_extensions to requirements
850a2a2
@jesper-friis
Merge branch '517-clean-up-requirements' of github.com:SINTEF/dlite i…
b492b74 
…nto 517-clean-up-requirements
@jesper-friis
Added typing_extensions to extra_requirements in setup.py
51e15c9
@jesper-friis
Added jsonschema<4.18 to avoid rust dependency. Lets see if it works...
399cc22
@jesper-friis
Install rust, cargo and poetry since Pydantic v2 seems to require them
8e8cc18 
when building wheels on some platforms...
@jesper-friis
Cleanded up ci_build_wheel
466741b
@jesper-friis
Install with apt-get
af332b4
@jesper-friis
Merge branch 'master' into 517-clean-up-requirements
317e12a
@jesper-friis
Only run apt-get if the command is available
67dacbf
@jesper-friis
Install rust the recommended way
1159649
@jesper-friis
Install rust non-interactively
9d32e07
@jesper-friis
Try to install rust via apt
95801ed
@jesper-friis
Fixed Python indentations
3ff5739
@jesper-friis
Formatting
8f502d5
@jesper-friis
Replace if with and in inline python expression
c965bb3
@jesper-friis
more formatting
2b4b97f
@jesper-friis
Use subprocess.run()
f221c22
@jesper-friis
Remove syntax error
67ed009
@jesper-friis
Also install cargo and change ' to " to also support windows
6437a63
@jesper-friis jesper-friis mentioned this pull request Aug 18, 2023
Closed
2 tasks
@jesper-friis jesper-friis changed the title WIP: Clean up requirements Clean up requirements Aug 18, 2023
@jesper-friis jesper-friis changed the title Clean up requirements Reduce requirements Aug 21, 2023
CasperWA
CasperWA requested changes Aug 21, 2023
View reviewed changes
Copy link
Collaborator

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

I have some changes - but to the original PR. I don't know what just happened but suddenly MANY files have been changed?

.github/docker/Dockerfile-manylinux.template Outdated Show resolved Hide resolved
.github/docker/Dockerfile-musllinux.template Outdated Show resolved Hide resolved
.github/docker/Dockerfile-musllinux.template Show resolved Hide resolved
.github/docker/Dockerfile-musllinux.template Outdated Show resolved Hide resolved
.github/docker/Dockerfile-musllinux.template Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
doc/getting_started/installation.md Show resolved Hide resolved
requirements_full.txt Outdated Show resolved Hide resolved
@CasperWA
Copy link
Collaborator

Black should not be run as part of this PR. That should be done in a separate PR.

@jesper-friis
Copy link
Collaborator Author

jesper-friis commented Aug 21, 2023 •
edited
Loading

Black should not be run as part of this PR. That should be done in a separate PR.

Black is not a part of this PR now. You were just unfortunate to look at it during the time-span between the black changes were applied to this branch and approved on master.

CasperWA
CasperWA requested changes Aug 22, 2023
View reviewed changes
Copy link
Collaborator

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

The requirements files in the dockerfiles are still wrongly copied.

.github/docker/Dockerfile-manylinux.template Outdated Show resolved Hide resolved
.github/docker/Dockerfile-musllinux.template Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
requirements_full.txt Outdated Show resolved Hide resolved
requirements_full.txt Outdated Show resolved Hide resolved
requirements_full.txt Outdated Show resolved Hide resolved
CasperWA
CasperWA requested changes Aug 24, 2023
View reviewed changes
.github/docker/Dockerfile-musllinux.template
COPY requirements.txt /tmp/requirements.txt
ARG PY_MINORS="7 8 9 10 11"

#COPY requirements.txt /tmp/requirements.txt
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#COPY requirements.txt /tmp/requirements.txt

Copy link
Collaborator

@CasperWA CasperWA Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You didn't want to remove this commented-out line?

requirements_full.txt Outdated Show resolved Hide resolved
@jesper-friis jesper-friis merged commit 899b14b into master Aug 24, 2023
10 checks passed
@jesper-friis jesper-friis deleted the 517-clean-up-requirements branch August 24, 2023 18:31
@jesper-friis jesper-friis mentioned this pull request Sep 12, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CasperWA CasperWA Awaiting requested review from CasperWA

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Clean up requirements
2 participants
@jesper-friis @CasperWA