Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove installation of build requirements from workflows, since they are already installed in the underlying docker images #2854

Merged
merged 3 commits into from
Nov 2, 2023

Conversation

francesco-ballarin
Copy link
Member

Turns out that most of this was already available in the docker files, but we did not trigger test/dev env workflow.
See https://github.com/FEniCS/dolfinx/actions/runs/6729006694

@francesco-ballarin
Copy link
Member Author

I need to manually rerun https://github.com/FEniCS/dolfinx/actions/workflows/docker-oneapi-test-env.yml and https://github.com/FEniCS/dolfinx/actions/workflows/docker-redhat-test-env.yml for this to work, but I'll wait doing that until the manual rerun by @jhale is finished.

@francesco-ballarin
Copy link
Member Author

francesco-ballarin commented Nov 2, 2023

All base images (ubuntu CI, oneapi and redhat) have been regenerated, and now all workflows are passing.

@@ -86,7 +86,6 @@ jobs:
python3 -m isort --check .
- name: mypy checks
run: |
python3 -m pip install mypy types-setuptools --upgrade # TO REMOVE
Copy link
Member Author

@francesco-ballarin francesco-ballarin Nov 2, 2023

Choose a reason for hiding this comment

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

The current workflow upgrades mypy at every run, i.e. if the version of mypy on ghcr.io/fenics/test-env:current-openmpi is old it will get upgraded.

From https://github.com/FEniCS/dolfinx/actions/workflows/docker-dev-test-env.yml I see that the docker-dev-test-env.yml workflow is manually triggered once a month or so, so I do not expect the version of mypy will ever be too old.

Copy link
Member

Choose a reason for hiding this comment

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

The workflows should not be using pip to install/upgrade dependencies or related tools. Everything should be in the appropriate Dockerfile.

Sometimes it's necessary to install something in a workflow to get the tests to pass and the PR through. The images should then be rebuilt and a cleanup PR created.

It's good practice to add a note e.g. # TO REMOVE as it's easy to forget.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only remaining pip install in these three workflows is https://github.com/FEniCS/dolfinx/blame/ef7255b655e96c88ef9556efa982df1c00987cf7/.github/workflows/redhat.yml#L27
which was just added 3 days ago. Do you want me to get rid of that as well?

Copy link
Member

Choose a reason for hiding this comment

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

The current workflow upgrades mypy at every run, i.e. if the version of mypy on ghcr.io/fenics/test-env:current-openmpi is old it will get upgraded.

From https://github.com/FEniCS/dolfinx/actions/workflows/docker-dev-test-env.yml I see that the docker-dev-test-env.yml workflow is manually triggered once a month or so, so I do not expect the version of mypy will ever be too old.

You're welcome to try. Would be interesting to see if redhat's pip is up to the task. If not we need to adjust the Dockerfile.

@jhale jhale added this pull request to the merge queue Nov 2, 2023
Merged via the queue into main with commit 938fa51 Nov 2, 2023
@jhale jhale deleted the francesco/docker-skbuild branch November 2, 2023 17:00
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants