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

clean up after dockerfile merge #575

Merged
merged 2 commits into from
May 22, 2023
Merged

Conversation

akihikokuroda
Copy link
Collaborator

Summary

This PR remove 2 unnecessary files for amd64 architecture related files and adjust some other files.

Details and comments

@@ -18,6 +18,8 @@ services:
build:
context: ./
dockerfile: infrastructure/docker/Dockerfile-ray-qiskit
args:
TARGETARCH: <Please set amd64 or arm64 here in the docker-compose-dev.yml>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use an envvar here, similar to how we set the version here: https://github.com/Qiskit-Extensions/quantum-serverless/blob/c761272e7c8cb3fb97d8fd356e95e0e9ece5e3b5/docker-compose.yml#L5

Also, and just thinking out loud here, are there other places where we set the arch that could use the same envvar? (ie if users could export DOCKERARCH=arm64 in their .bashrc and then not have to worry about it anymore...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@psschwei Thanks for review. I couldn't make the environment variable work so I put the the message there to show the message in the error when the user uses it first time.

We can use the uname -m for must of the cases to get the platform architecture. I just couldn't make the docker compose work with any variables. :-(

Copy link
Collaborator

Choose a reason for hiding this comment

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

This works for x86: TARGETARCH: "${TARGETARCH:-amd64}". Could you try that and run with TARGETARCH=arm64 docker compose...?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! The reason that it doesn't work for me may be I'm using nerdctl on mac. I put the env variable here for the most of users.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if you need to declare it as an ARG in the dockerfile and/or explicitly pass it as an environmental variable to compose ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it actually causes a problem in the github action: docker/buildx#574

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
@akihikokuroda akihikokuroda merged commit 98b7687 into Qiskit:main May 22, 2023
@akihikokuroda akihikokuroda deleted the cleanup branch May 22, 2023 15:58
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.

2 participants