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

284 add support for arm architecture for singularity build #286

Merged

Conversation

munishchouhan
Copy link
Member

@munishchouhan munishchouhan commented Aug 23, 2023

add arm architecture support for singularity builds using --platform flag

@munishchouhan munishchouhan linked an issue Aug 23, 2023 that may be closed by this pull request
@munishchouhan munishchouhan self-assigned this Aug 23, 2023
@munishchouhan munishchouhan marked this pull request as draft August 23, 2023 11:48
@munishchouhan
Copy link
Member Author

Singularity 4 docker is out, I will move forward with this PR
https://quay.io/repository/singularity/singularity

@munishchouhan munishchouhan marked this pull request as ready for review October 24, 2023 07:50
@munishchouhan
Copy link
Member Author

we have platform in two places while building singularity image
--platform linux/arm64 quay.io/singularity/singularity:v4.0.1-slim sh -c singularity build image.sif /Users/munish.chouhan/main_ground/wave/284/wave/build-workspace/93569c0c19462261/Containerfile --platform linux/arm64

@pditommaso should i create another field in buildrequest to specify singularity platform?

@pditommaso
Copy link
Collaborator

Ideally this should be enough

@munishchouhan
Copy link
Member Author

@pditommaso so we should use singularity image when the platform is arm46

@marcodelapierre
Copy link
Contributor

I think multi-arch builds are not supported yet in Singularity 4.0.

The user guide only mentions support for pulling architecture-/platform- specific containers:
https://docs.sylabs.io/guides/4.0/user-guide/singularity_and_docker.html#specifying-a-platform-architecture

The user case for this feature seems to be to provide emulation support to run containers on a certain arch, that were built for a different arch. (I can think e.g. of those x86_64 only containers, that someone may need to run on ARM, RISC or other).

So for now the only available way for multi-arch seems to be docker builds.

@marcodelapierre
Copy link
Contributor

Evoking the one and mighty Vanessasaurus @vsoch :

are multi arch builds with Singularity currently under consideration?

(I browsed issues / discussions in the sylabs singularity repo, but could not find mention of it)

@marcodelapierre
Copy link
Contributor

marcodelapierre commented Oct 30, 2023

[update] after some additional study on the command line (still undocumented), I see that singularity build has indeed got --arch and --platform options. The use case, as inferred by the CLI help screen, is to pick the build architecture when using Sylabs remote build service.

However, the machine on which singularity is running needs to have matching architecture.

So I have two questions @munishchouhan :

  1. I assume that your current implementation exploit the Wave server capability to route the builds on machines of appropriate architecture, right?

  2. if 1. is the case, do we need the --arch flag at all? I would assume (haven't tested it yet myself) that a Singularity build on an ARM machine would build an ARM container by default, right?

Thanks,

@munishchouhan
Copy link
Member Author

Is the support for K8s missing, isn't it? if so please mark draft until isn't complete

This feature needs two changes, one is singularity arm image, and arm node

  1. I have made changes for singularity arm image
  2. Node selection is already there

Copy link
Contributor

@marcodelapierre marcodelapierre left a comment

Choose a reason for hiding this comment

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

One more request:

please add appropriate testing for the K8s case in src/test/groovy/io/seqera/wave/service/builder/KubeBuildStrategyTest.groovy

@munishchouhan
Copy link
Member Author

One more request:

please add appropriate testing for the K8s case in src/test/groovy/io/seqera/wave/service/builder/KubeBuildStrategyTest.groovy

done

Copy link
Contributor

@marcodelapierre marcodelapierre left a comment

Choose a reason for hiding this comment

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

good to go for me

Comment on lines 56 to 57
@Value('${wave.build.singularity-image-arm}')
String singularityImageArm
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
@Value('${wave.build.singularity-image-arm}')
String singularityImageArm
@Value('${wave.build.singularity-image-arm64}')
String singularityImageArm64

Let's rename to arm64 suffix for consistency.

Also, this can be made nullable, and the default to singularityImage + '-arm64'

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe i'm wrong here, but it looks like that when singularityImageArm64 is not specified null it will be used.

My suggestion was instead that when singularityImageArm64 is not specied (nullable) the value of singularityImage is appending to it the -arm64, suffix.

To avoid duplicating the code, it could make sense to introduce an object BuildConfig (along the same way of SpackConfig) that holds all the wave.build.* settings

Copy link
Member Author

Choose a reason for hiding this comment

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

But in this way, we are coupling our code with the naming convention of singularity docker image. If somewhere down the line they change this, then our code will be broken

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

ok i understood now, thanks for the clarification

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@marcodelapierre
Copy link
Contributor

@munishchouhan @pditommaso how are we going here?

@munishchouhan
Copy link
Member Author

@marcodelapierre everything is done from my side

@marcodelapierre
Copy link
Contributor

@pditommaso when you get a chance please have a look at Munish' commits that tackle your request

munishchouhan and others added 3 commits November 7, 2023 10:35
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
@pditommaso
Copy link
Collaborator

For sake of simplicity I've removed the @nullable on arm64 image name. Let's implement it in the BuildConfig object

@pditommaso pditommaso merged commit d4d2748 into master Nov 7, 2023
3 checks passed
@pditommaso pditommaso deleted the 284-add-support-for-arm-architecture-for-singularity-build branch November 7, 2023 11:21
pditommaso added a commit that referenced this pull request Nov 10, 2023

Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
Co-authored-by: Munish Chouhan <hrma017@gmail.com>
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.

Add support for ARM architecture for Singularity build
4 participants