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

Adding first shot at new features (x11 and fake home) #444

Merged
merged 4 commits into from
Sep 23, 2021
Merged

Conversation

vsoch
Copy link
Member

@vsoch vsoch commented Sep 4, 2021

this first commit will add a new feature, x11, which will be null by default. If the value is True, we default to binding ~/.Xauthority. if it is found to be a string, we use that instead. This means if there needs to be some custom bind like ~/.special-Xauthority TO the ~/.Xauthority, both of those need to be represented in the single string.

This will close #433

@marcodelapierre I wanted to just do this one first in case its implementation influences our decision making for the fake home. I also had a few questions about the fake home:

  1. why not just call it home?
  2. Singularity would need --home and the bind, if I understand you correctly?
  3. Akin to the Xauthority here, it would be up to the center / settings writer to either provide a single path (and to bind to and from using it) OR to specify a <source>:<dest> for the bind instead.

I also added some weird syntax that basically says "if the type of the value specified is within one we know to allow (e.g., "~/.my-custom-Xauthority is a string and str is in the features lookup for x11:

    features = {
        "gpu": {"nvidia": "--nv", "amd": "--rocm"},
        "x11": {True: "~/.Xauthority", str: "[use-self]"},
    }

AND if the value is [use-self] in that lookup, just use the original string. This also gives us a little wiggle room for custom parsing of strings provided by the user to indicate some value. I'm hoping we don't need to get more complicated than this though, e.g., "if I find a string for x11, just use it"

Signed-off-by: vsoch vsoch@users.noreply.github.com

this first commit will add a new feature, x11, which will be null by default. If the
value is True, we default to binding ~/.Xauthority. if it is found to be a string, we use that
instead. This means if there needs to be some custom bind like ~/.special-Xauthority TO the ~/.Xauthority,
both of those need to be represented in the single string.

Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@marcodelapierre
Copy link
Contributor

marcodelapierre commented Sep 10, 2021

Hi @vsoch , here I am! :-)

  1. agree, home makes sense
  2. I...honestly was not aware of the --home flag ...! I did a quick test, basically for home you can just have --home <src>:<dest>, which will also perform the binding, so that --B is not even needed. In addition, --home also redefined the HOME variable accordingly, so... agree, you should use this powered-up flag!
  3. I like this implementation. Question: the string implementation would then work with both <path> and <src>:<dest> syntaxes, right?

Edit: for the x11 setting, I would also allow a False value, in case the site does not allow these types of setups, in which case the container module would add no flag ...?

@vsoch
Copy link
Member Author

vsoch commented Sep 10, 2021

Yes!

I like this implementation. Question: the string implementation would then work with both and : syntaxes, right?

Yes.

Edit: for the x11 setting, I would also allow a False value, in case the site does not allow these types of setups, in which case the container module would add no flag ...?

The default is null, but also false can be set (and it's the same as null, no flag added)

@marcodelapierre
Copy link
Contributor

... and I just realised, that very same setup can be used for the 'home' feature, too. With True defaulting to $HOME.
False/null can be useful for sites that already mount home by default, so usage of this feature is not needed.

@vsoch
Copy link
Member Author

vsoch commented Sep 10, 2021

Wait, so which one is not needed? Can you be more explicit so I know what to update (remove, add change)?

@marcodelapierre
Copy link
Contributor

marcodelapierre commented Sep 10, 2021

For the Home feature only, to get the host directory mounted as home there seem to be 2 options in Singularity (alternative path/flag syntaxes omitted for brevity):

  1. --home <src>:<path>: mounts <src> as <dst> and sets HOME=<dst>
  2. -B <src>:<path>: only mounts <src> as <dst>

So if you use flag 1. for the Home feature, you don't need flag 2.

@vsoch
Copy link
Member Author

vsoch commented Sep 10, 2021

Yes correct! I was planning on implementing 1., does that work?

@marcodelapierre
Copy link
Contributor

Yes, I confirm it works!

Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@vsoch vsoch changed the title Adding first shot at new features (starting with x11) Adding first shot at new features (x11 and fake home) Sep 12, 2021
@vsoch
Copy link
Member Author

vsoch commented Sep 12, 2021

okey doke, fake home is added to the PR! We probably could better document feat it we at this point, e.g gpu and x11 are only singularity. Maybe a simple table?

@marcodelapierre
Copy link
Contributor

marcodelapierre commented Sep 14, 2021

Tables are a great idea!

Some comments:

  1. how about having a similar API for home, compared to x11? i.e. also having a true/false config option in the shpc settings, with true defaulting to "$HOME"?

  2. committed changes to docs/getting_started/user-guide.rst: just heads up that right now, the "gpu" feature is discussed in 2 distinct documentation places.
    The User Guide focuses on the shpc settings, see https://singularity-hpc.readthedocs.io/en/latest/getting_started/user-guide.html#features ; it could also mention more in general that this is controlled by a true/false property in the container recipe ... and this could also become an additional column in the current table in that page, as you say.
    The Developer Guide mentions the true/false options for the container recipes: https://singularity-hpc.readthedocs.io/en/latest/getting_started/developer-guide.html#registry-yaml-fields .

@vsoch
Copy link
Member Author

vsoch commented Sep 14, 2021

how about having a similar API for home, compared to x11? i.e. also having a true/false config option in the shpc settings, with true defaulting to "$HOME"?

Singularity already defaults to binding $HOME to the container home, so this would not be necessary (or would be weirdly redundant).

committed changes to docs/getting_started/user-guide.rst: just heads up that right now, the "gpu" feature is discussed in 2 distinct documentation places.
The User Guide focuses on the shpc settings, see https://singularity-hpc.readthedocs.io/en/latest/getting_started/user-guide.html#features ; it could also mention more in general that this is controlled by a true/false property in the container recipe ... and this could also become an additional column in the current table in that page, as you say.
The Developer Guide mentions the true/false options for the container recipes: https://singularity-hpc.readthedocs.io/en/latest/getting_started/developer-guide.html#registry-yaml-fields .

Ack totally forgot about that! I'll update the PR this evening.

Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@vsoch
Copy link
Member Author

vsoch commented Sep 14, 2021

okay added a table of just features to the developer guide! a37281a

@marcodelapierre
Copy link
Contributor

Singularity already defaults to binding $HOME to the container home, so this would not be necessary (or would be weirdly redundant).

Oh you're right -- I am biased here because we disable default home binding in our HPC clusters.

@vsoch
Copy link
Member Author

vsoch commented Sep 16, 2021

okay added a table of just features to the developer guide! a37281a. Sorry wrote this but it didn't post!

@marcodelapierre
Copy link
Contributor

marcodelapierre commented Sep 22, 2021

Hi @vsoch ,

I have done some testing using Singularity for both x11 and home : all good for me!

A couple of comments:

  1. for consistency, I would edit the user guide, line 261 of file docs/getting_started/user-guide.rst:
    current: In settings, should be null or a path for the home. In the container config, can be true or false (default if not present)
    suggested: custom path for the home, or false/null

  2. also, line 259 of the same file:
    current: Specify a custom home. Should be true/false in container config, and a string path in settings
    suggested: Specify and bind mount a custom home path

  3. Could you add home support for me in this container yaml? registry/quay.io/biocontainers/beast2/container.yaml - thanks!

features:
  home: true

Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@vsoch
Copy link
Member Author

vsoch commented Sep 22, 2021

All set! 48515de

@marcodelapierre
Copy link
Contributor

Cool! Can't wait to deploy on our upcoming Supercomputer ;)

@vsoch
Copy link
Member Author

vsoch commented Sep 23, 2021

Ready for merge and release then?

@marcodelapierre
Copy link
Contributor

I'd say so! :)

@vsoch vsoch merged commit 1004081 into main Sep 23, 2021
@vsoch vsoch deleted the add/new-features branch September 23, 2021 01:36
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.

New "Feature": X11 applications
2 participants