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

Jupyter Interactive Tool 1.0.1/24.07 #18861

Merged
merged 2 commits into from
Nov 12, 2024
Merged

Conversation

natefoo
Copy link
Member

@natefoo natefoo commented Sep 20, 2024

Note: You need to set $NB_UID in your job conf environment (destination) for this tool.

docker_run_extra_arguments: "-e NB_UID=$(id -u)"

I added <required_files> for running via Pulsar but this is otherwise just a version update from tools/interactive/interactivetool_jupyter_notebook_1.0.0.xml.

How to test the changes?

  • This is a refactoring of components with existing test coverage.

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@natefoo
Copy link
Member Author

natefoo commented Sep 20, 2024

I just realized maybe we prefer to update the 1.0.0 tool instead since it is functionally the same (and the functional difference is why I assume 0.3 and 1.0.0 were made separate in the first place).

@github-actions github-actions bot added this to the 24.2 milestone Sep 20, 2024
@mvdbeek
Copy link
Member

mvdbeek commented Sep 20, 2024

Note: You need to set $NB_UID in your job conf environment (destination) for this tool.

Why is that, and can we avoid this ?

@natefoo
Copy link
Member Author

natefoo commented Sep 20, 2024

@bgruening correct me if I'm wrong, but the upstream Jupyter/Julia image on which it's based starts as root and drops privileges to $NB_UID. If unset then jupyter runs as jovyan and can't write to the working dir. To change this would require building fully custom images.

@mvdbeek
Copy link
Member

mvdbeek commented Sep 20, 2024

That seems like a much better idea to me, if NB_UID can't be set to 0.

@natefoo
Copy link
Member Author

natefoo commented Sep 20, 2024

Building fully custom images? That is a lot of work to keep updated since we can't just build off the already maintained upstream images.

@mvdbeek
Copy link
Member

mvdbeek commented Sep 20, 2024

Yeah, but you can't write in your PR "oh and btw put this in your job conf" and expect it'll be done without trial and error. So now we could build out an abstraction for this ... or we do the "container thing" of not dropping privileges. Surely you can overwrite that one script that drops privileges ?

@mvdbeek
Copy link
Member

mvdbeek commented Sep 20, 2024

(I'd also check if NB_UID=0 works)

@natefoo
Copy link
Member Author

natefoo commented Sep 20, 2024

That might work depending on the deployment, but not for root-squashed filesystems, and would make cleanup more difficult.

@mvdbeek
Copy link
Member

mvdbeek commented Sep 20, 2024

I admit I'm not an admin, but aren't you always 0 in a standard container and your container engine manages the id translation ?

❯ docker run quay.io/biocontainers/galaxy-data:24.1.1--pyhdfd78af_0 id
uid=0(root) gid=0(root) groups=0(root)

Are you saying the problem is not that this joyvan image is trying to be a small VM when it shouldn't ?

@natefoo
Copy link
Member Author

natefoo commented Sep 20, 2024

You are root unless you 1. pass --user or 2. the container's CMD or ENTRYPOINT do something to change users, which is not uncommon among containers that run fairly complex applications.

@mvdbeek
Copy link
Member

mvdbeek commented Sep 20, 2024

You are root unless you 1. pass --user or 2. the container's CMD or ENTRYPOINT do something to change users, which is not uncommon among containers that run fairly complex applications.

That is a good point, however then I would set $NB_UID=$(id) in the container

he container's CMD or ENTRYPOINT do something to change users,

but I'm suggesting to just not do that, which seems feasible for this case ? Let's avoid complexity where we can, and otherwise I think we should at least have language to say "This tool requires the following environment variable to be set"

@natefoo
Copy link
Member Author

natefoo commented Sep 20, 2024

That is a good point, however then I would set $NB_UID=$(id) in the container

afaik we can't without modifying the upstream container.

but I'm suggesting to just not do that, which seems feasible for this case ? Let's avoid complexity where we can, and otherwise I think we should at least have language to say "This tool requires the following environment variable to be set"

I presume there is a reason why they do this, people more familiar with the upstream image probably know why. I am happy to document it but we still don't really have a way for tool authors to provide documentation to admins, do we?

@natefoo
Copy link
Member Author

natefoo commented Oct 1, 2024

@mvdbeek is there something you'd like me to do here? FWIW this is not a change from the behavior of the existing 1.x version of this tool.

@@ -1,7 +1,10 @@
<tool id="interactive_tool_jupyter_notebook" tool_type="interactive" name="Interactive JupyterLab Notebook" version="1.0.0" profile="22.01">
<tool id="interactive_tool_jupyter_notebook" tool_type="interactive" name="Interactive JupyterLab Notebook" version="1.0.1" profile="23.0">
Copy link
Member

Choose a reason for hiding this comment

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

Can you copy the file ? We have no other way to version ITs at the moment.

@mvdbeek mvdbeek merged commit e34f6ad into galaxyproject:dev Nov 12, 2024
49 of 51 checks passed
Copy link

This PR was merged without a "kind/" label, please correct.

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.

3 participants