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

[sirius] GH200 recipe #96

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[sirius] GH200 recipe #96

wants to merge 3 commits into from

Conversation

toxa81
Copy link
Collaborator

@toxa81 toxa81 commented May 17, 2024

No description provided.

@toxa81 toxa81 self-assigned this May 21, 2024
Copy link
Member

@RMeli RMeli left a comment

Choose a reason for hiding this comment

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

A few comments, mainly based on comments I received on my recipes. Documentation is currently missing, it should be added to docs/uenv-sirius.md.

Comment on lines +6 to +7
# commit on develop that updated the sirius package.py for v7.5.0 of sirius
commit: 13b711f
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to use a released version of Spack (0.22.0 is now out), and add the SIRIUS package in the repo/ folder if needed? I think this is the preferred way to do things.

Copy link
Member

Choose a reason for hiding this comment

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

This is two topics:

  • we should add support for Spack v0.22.0 to stackinator
  • a specific commit like this is better than master (because it is more reproducible)
    • but using a tagged release (e.g. v0.21.2) or release branch (e.g. releases/v0.21) is better
    • so moving the sirius package to a repo path is preferable, if possible.

Comment on lines +27 to +28
views:
default:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
views:
default:
views:
sirius:
link: root

view:link:root is recommended in the docs.

Do you expect users to build SIRIUS from source themselves? If yes, we need a develop view too, where SIRIUS is excluded. This will likely require some tinkering on which specs do you need to add explicitly to the view so that they are available.

It is not yet documented, but the procedure to compile from source should be added to the documentation.

Copy link
Member

Choose a reason for hiding this comment

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

I'd skip the develop environment here. The "general public" won't build sirius, and those who do will know how to use spack upstream.

Copy link
Member

Choose a reason for hiding this comment

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

Given the number of root specs (and a few patches) I had to add to #95, I totally agree.

However, from a private conversation with @bcumming it seemed that a develop view is still preferred. I'll let him comment further.

Copy link
Member

Choose a reason for hiding this comment

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

I have had a bit of a think about this, and how about the following:

The interfaces that you choose to provide (spack, filesystem view, modules) depend on the user community and their needs, and how the uenv maintainer intends to provide support to the users.

If you have a community that only use Spack, then you might not need views or modules for development at all.

If you have some users who will insist on "building their own" using a view or modules, then configure them in a way that will minimise your maintenance burden.

Whatever interfaces are chosen, they should be properly documented in the /docs/<uenv-name>.md docs.

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

Successfully merging this pull request may close these issues.

4 participants