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

adjusting README and Dockerfile for x86_64 and arm64 usage #22

Merged
merged 8 commits into from
Apr 22, 2024

Conversation

Urfoex
Copy link
Contributor

@Urfoex Urfoex commented Mar 4, 2024

No description provided.

@Urfoex Urfoex requested review from alxn4, liuzicheng1987, srnnkls and a team March 4, 2024 21:20
@Urfoex Urfoex changed the title adjusting path to requirements.txt in README adjusting README and Dockerfile for x86_64 and arm64 usage Mar 11, 2024
@Urfoex
Copy link
Contributor Author

Urfoex commented Mar 11, 2024

Solves #19

@alxn4
Copy link
Member

alxn4 commented Mar 18, 2024

Thanks for your updates! Can we add a more meaningful error message when we try to spin up the image on macos or windows host system? Right now I am receiving the following message:

 ~/code17/getml-demo  update_readme_1_4_0 ?1  docker-compose up notebooks --build                                          17 err  23:37:21
[+] Building 0.0s (1/1) FINISHED                                                                                        docker:desktop-linux
 => [notebooks internal] load build definition from Dockerfile                                                                          0.0s
 => => transferring dockerfile: 959B                                                                                                    0.0s
failed to solve: failed to parse platform : "" is an invalid component of "": platform specifier component must match "^[A-Za-z0-9_-]+$": invalid argument

On a minor note, please use the <randomly_generated_token> placeholder consistently.

@srnnkls
Copy link
Collaborator

srnnkls commented Mar 19, 2024

This should just run on macOS and there should be no error at all. Will have a look in a few minutes.

The error indicates that the build arg is not populated on macOS.

@srnnkls
Copy link
Collaborator

srnnkls commented Mar 19, 2024

The problem here is the redefinition of the TARGETPLATFORM arg in line 1 of the Dockerfile. Just remove it and everything should work as expected.

The use of build args in docker is a little tricky:
If you use the ARG directive outside of build stages you are declaring the arg, if you use it inside a build stage, you are making the arg defined outside visible to the stage. In this case the arg was prepopulated, so no need to declare it. And as you are reference it only from the outside (in the FROM directive) you don't need to make it visible to the build stage at all.

@srnnkls
Copy link
Collaborator

srnnkls commented Mar 19, 2024

But was it actually working with podman, @Urfoex?

@srnnkls
Copy link
Collaborator

srnnkls commented Mar 19, 2024

On a minor note, please use the <randomly_generated_token> placeholder consistently.

I would add that the users shouldn't be concerned about the stochastic nature of the token at all: Just name it <token>.

Copy link
Collaborator

@srnnkls srnnkls left a comment

Choose a reason for hiding this comment

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

Some small comments. Otherwise LGTM. Great work!

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@Urfoex
Copy link
Contributor Author

Urfoex commented Mar 22, 2024

But was it actually working with podman, @Urfoex?

Yeah, I guess podman sees "These arguments are defined in the global scope" differently then docker.
But the TARGETPLATFORM is not needed here anyway, as "By default, the target platform of the build request is used."
So I threw it out.

On a minor note, please use the <randomly_generated_token> placeholder consistently.

Adjusted.

I would add that the users shouldn't be concerned about the stochastic nature of the token at all: Just name it .

I put <generated_token> to distinguish it from token in the monitor part, as that is set by hand. And <token> and token just look too similar and might confuse.

…ker, simplify by removing multiarch parts. Using heredoc for multiline RUN
@srnnkls
Copy link
Collaborator

srnnkls commented Apr 3, 2024

lgtm. have a good merge!

.env Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@Urfoex Urfoex merged commit 323bf33 into master Apr 22, 2024
@Urfoex Urfoex deleted the update_readme_1_4_0 branch April 22, 2024 12:02
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.

4 participants