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

Create new docker image for linux adapters. #8270

Closed
wants to merge 1 commit into from

Conversation

kgpai
Copy link
Contributor

@kgpai kgpai commented Jan 5, 2024

This PR creates a docker image which uses our default circleci script as base but also adds minio and jdk on top. This will allow us to not download mino / install jdk for every build.

Copy link

netlify bot commented Jan 5, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 2fa8dfe
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/659dc4052882e00008fe9776

@kgpai kgpai requested a review from assignUser January 5, 2024 01:11
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 5, 2024
@kgpai kgpai requested a review from majetideepak January 5, 2024 01:12
# or
# docker-compose run -e NUM_THREADS=<NUMBER_OF_THREADS_TO_USE> --rm linux-adapters
# to set the number of threads used during compilation
image: ghcr.io/facebookincubator/velox-dev:amd64-centos-8-avx
Copy link
Collaborator

Choose a reason for hiding this comment

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

the name needs to be updated as well (we can look at overhauling our images as well to make maintenance easier...)

Comment on lines 16 to 20
FROM quay.io/centos/centos:stream8
ARG cpu_target
COPY scripts/setup-circleci.sh /
COPY scripts/setup-helper-functions.sh /
RUN mkdir build && ( cd build && CPU_TARGET="$cpu_target" bash /setup-circleci.sh ) && rm -rf build
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why build everything twice? ^^

Suggested change
FROM quay.io/centos/centos:stream8
ARG cpu_target
COPY scripts/setup-circleci.sh /
COPY scripts/setup-helper-functions.sh /
RUN mkdir build && ( cd build && CPU_TARGET="$cpu_target" bash /setup-circleci.sh ) && rm -rf build
FROM ghcr.io/facebookincubator/velox-dev:circleci-avx

We would also want to add this to the docker.yml workflow to automatically update it but probably as a second step with needs: to the matrix build so it uses the new image... not optimal but eh...

@mbasmanova
Copy link
Contributor

@kgpai It would be nice to create a GitHub issue to describe the problem being solved and attach this PR to it.

@kgpai
Copy link
Contributor Author

kgpai commented Jan 9, 2024

@assignUser Can you have another look, I am not sure how to use the 'needs:' tag you were talking about , my very brief survey in the docker docs (https://docs.docker.com/compose/compose-file/05-services/#depends_on) . Thanks.

@assignUser
Copy link
Collaborator

Ah sorry for the confusion. needs: is a Github Actions tag that makes a job depend on another job. In the docker.yml workflow we would want to build and upload the circleci-avx image before we use it in this build. As we use ghcr.io we don't have to handle artifacts etc but can just let docker deal with it :)

@majetideepak
Copy link
Collaborator

@kgpai should we include DuckDB as well?

@assignUser
Copy link
Collaborator

Superseeded by 8734

@assignUser assignUser closed this Mar 13, 2024
facebook-github-bot pushed a commit that referenced this pull request Mar 20, 2024
Summary:
This PR moves the following jobs to gha:
- adapters
- presto fuzzer run
- linux build + short fuzzer

I have integrated the short 60s normal and extended 1800s presto fuzzer into the existing workflow in `scheduled.yml` (which we probably want to rename to `fuzzer.yml` ^^).

I have added new docker files for the adapters job so we can close #8270 kgpai .

In a follow up pr I will add the signature check and bias fuzzer to the `ubuntu-debug` job. Additionally I will use the new centos docker file to create an image based on manylinux for #9014 .

Pull Request resolved: #8734

Reviewed By: Yuhta

Differential Revision: D55097698

Pulled By: kgpai

fbshipit-source-id: 55fa23c23cde97dce6f035b8e8f229608f6df422
Joe-Abraham pushed a commit to Joe-Abraham/velox that referenced this pull request Jun 7, 2024
…r#8734)

Summary:
This PR moves the following jobs to gha:
- adapters
- presto fuzzer run
- linux build + short fuzzer

I have integrated the short 60s normal and extended 1800s presto fuzzer into the existing workflow in `scheduled.yml` (which we probably want to rename to `fuzzer.yml` ^^).

I have added new docker files for the adapters job so we can close facebookincubator#8270 kgpai .

In a follow up pr I will add the signature check and bias fuzzer to the `ubuntu-debug` job. Additionally I will use the new centos docker file to create an image based on manylinux for facebookincubator#9014 .

Pull Request resolved: facebookincubator#8734

Reviewed By: Yuhta

Differential Revision: D55097698

Pulled By: kgpai

fbshipit-source-id: 55fa23c23cde97dce6f035b8e8f229608f6df422
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants