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.

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

Update dockerfile setup to use mamba #583

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

ayushdg
Copy link
Collaborator

@ayushdg ayushdg commented Jun 16, 2022

With packaging updates for different min versions in dask-sql the conda solver fails to solve properly in our dockerfile setup. Switching to mamba seems to resolve this.

@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2022

Codecov Report

Merging #583 (3c6be77) into main (0db4506) will increase coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #583      +/-   ##
==========================================
+ Coverage   88.45%   88.60%   +0.14%     
==========================================
  Files          69       69              
  Lines        3500     3500              
  Branches      707      707              
==========================================
+ Hits         3096     3101       +5     
+ Misses        317      308       -9     
- Partials       87       91       +4     
Impacted Files Coverage Δ
dask_sql/_version.py 34.00% <0.00%> (+1.44%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@jdye64
Copy link
Collaborator

jdye64 commented Jun 17, 2022

@ayushdg this is marked as a draft but seems ready. Are you ready for a review?

@ayushdg
Copy link
Collaborator Author

ayushdg commented Jun 17, 2022

@ayushdg this is marked as a draft but seems ready. Are you ready for a review?
Not yet. The core issues is fixed but I'm also trying to update GitHub actions to test docker changes if those files are touched and I need to resolve some issues there.

Copy link
Collaborator

@charlesbluca charlesbluca left a comment

Choose a reason for hiding this comment

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

We can probably consolidate the checks on whether or not to push the docker images by adding something like this to the job itself:

  push_to_registry:
    name: Push Docker image to Docker Hub
    runs-on: ubuntu-latest
    env:
      DOCKER_PUSH: ${{ github.event_name == 'push' && github.repository == 'dask-contrib/dask-sql' }}
    if: github.repository == 'dask-contrib/dask-sql'

Don't think we need to check the github.ref condition as the on.push condition should ensure that it is always true if github.event_name == 'push'.

Also think we might be able to remove theif condition from the job itself? Assuming that we would only be pushing Docker images on pushes to this repo, we don't really lose much by having this extra CI running on forks.

.github/workflows/docker.yml Outdated Show resolved Hide resolved
@@ -44,7 +51,8 @@ jobs:
platforms: linux/amd64,linux/arm64,linux/386
tags: ${{ steps.docker_meta_main.outputs.tags }}
labels: ${{ steps.docker_meta_main.outputs.labels }}
push: true
push: ${{ github.ref == 'refs/head/main' && github.repository == 'dask-contrib/dask-sql' }}
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
push: ${{ github.ref == 'refs/head/main' && github.repository == 'dask-contrib/dask-sql' }}
push: ${{ env.DOCKER_PUSH }}

.github/workflows/docker.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@charlesbluca charlesbluca left a comment

Choose a reason for hiding this comment

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

Based on internal discussion, we don't want to load when we are pushing images to multiple platforms, and we also only want to build for a single platform when running on PRs.

.github/workflows/docker.yml Outdated Show resolved Hide resolved
.github/workflows/docker.yml Outdated Show resolved Hide resolved
.github/workflows/docker.yml Outdated Show resolved Hide resolved
@jdye64
Copy link
Collaborator

jdye64 commented Jun 23, 2022

This isn't my area of expertise but wanted to say thank you @ayushdg because I will definitely be consuming this and benefiting!

@ayushdg
Copy link
Collaborator Author

ayushdg commented Jun 28, 2022

Going through the errors. It looks like docker cloud step still seems to be trying to fetch some image information from docker hub instead of using the image that is present locally (loaded at the previous step). Pinging @jacobtomlinson in case you have any suggestions on how we can use the image generated in the main step for a subsequent step without pushing to docker hub.

tags: ${{ steps.docker_meta_cloud.outputs.tags }}
labels: ${{ steps.docker_meta_cloud.outputs.labels }}
push: true
push: ${{ fromJSON(env.DOCKER_PUSH) }}

Choose a reason for hiding this comment

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

I'm not 100% sure but I think you might need to change this to be load: true in order for it to pick up the local base image. Then add an additional push step. Setting load: true changes the behaviour of the builder.

See https://stackoverflow.com/a/63927832/1003288

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants