#492 #493

JoeyC12 · 2024-07-01T23:07:22Z

‘garden-ai hpc-notebook start --working-directory $(pwd)/tmp --notebooks-dir $(pwd)/notebooks’ starts a Jupyter notebook in an Apptainer Container.

📚 Documentation preview 📚: https://garden-ai--493.org.readthedocs.build/en/493/

codecov-commenter · 2024-07-01T23:10:22Z

Codecov Report

Attention: Patch coverage is 15.00000% with 51 lines in your changes missing coverage. Please review.

Project coverage is 74.06%. Comparing base (1d34837) to head (c569100).
Report is 24 commits behind head on main.

Files with missing lines	Patch %	Lines
garden_ai/app/hpc_notebook.py	15.00%	51 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #493      +/-   ##
==========================================
- Coverage   75.50%   74.06%   -1.45%     
==========================================
  Files          55       56       +1     
  Lines        3695     4288     +593     
==========================================
+ Hits         2790     3176     +386     
- Misses        905     1112     +207

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

WillEngler

This is a solid start, good job getting Jupyter running from inside Singularity/Apptainer. The biggest issue to address is making sure that the user can run this command from outside the Garden repo

WillEngler · 2024-07-02T16:28:50Z

Singularity.def

+
+%startscript
+
+    exec /bin/bash -c "jupyter notebook --notebook-dir=/notebooks --ip 0.0.0.0 --no-browser --allow-root"


Why do we need allow-root here?

WillEngler · 2024-07-02T16:33:32Z

garden_ai/app/hpc_notebook.py

+    try:
+        # Ensure the definition file exists
+        if not os.path.isfile(definition_file):
+            logger.error(f"Definition file {definition_file} not found.")


Does this approach rely on the user being in the root directory of the project where Singularity.def is? Let's make sure the user can run this command in any directory.

WillEngler · 2024-07-02T16:35:12Z

garden_ai/app/hpc_notebook.py

+        # Step 3: Build the Apptainer container
+        build_command = ["apptainer", "build", container_image, definition_file]
+        subprocess.run(build_command, check=True)
+        logger.info("Apptainer container built successfully.")


Does this command give you a way to reuse an image if you've already built it?

Yes, I add a rerun command

WillEngler · 2024-07-02T16:36:54Z

Singularity.def

@@ -0,0 +1,25 @@
+Bootstrap: docker


This sort of file shouldn't live at the root of the project. Our Docker building script is in scripts. Can you move this there or create a new appropriate directory for it?

WillEngler · 2024-07-02T16:38:02Z

notebooks/notebooks.md

@@ -0,0 +1 @@
+Place to store notebooks


If the intent is that this is for users' notebook files, it doesn't belong in the project repository.

WillEngler · 2024-07-02T16:38:07Z

tmp/tmp.md

@@ -0,0 +1 @@
+Place to store temporary file


Similarly, a tmp directory for the user should not be committed to the project.

WillEngler · 2024-07-02T16:48:26Z

garden_ai/app/hpc_notebook.py

+            "notebook",
+            "--no-browser",
+            "--ip=0.0.0.0",
+            "--allow-root",


It looks like this command will use the default Jupyter port. I'd recommend reusing the find_available_port helper to avoid the (likely) scenario of colliding with another user on the login node using the default Jupyter port

WillEngler · 2024-07-02T16:50:38Z

garden_ai/app/hpc_notebook.py

+        typer.echo("🚧🌱🚧 Failed to start Jupyter Notebook 🚧🌱🚧")
+    except Exception as e:
+        logger.error(f"An error occurred: {e}")
+        typer.echo("🚧🌱🚧 An unexpected error occurred 🚧🌱🚧")


How should the user shut down their Singularity container when they're done? It's ok to not build in really robust cleanup handling right now, but ideally we could handle the happy path of the user hitting ctrl-c. Or at least we should print instructions to the user for how they can manually end their session.

@JoeyC12 Can you address this question?

… Move definition file to scripts and use absolute path.

WillEngler · 2024-07-12T16:33:20Z

garden_ai/app/hpc_notebook.py

+):
+    root = os.path.dirname(os.path.abspath(__file__))
+    root = os.path.dirname(root)
+    root = os.path.dirname(root)


I think saving the sif file next to where the user has installed the Python code is pretty surprising. That will make it hard to find and clean up. Can we instead save it by default in the directory where the user ran the garden-ai command? (Maybe with an option to specify a different directory?)

Also it looks like there are some repeated lines here

WillEngler · 2024-07-12T16:36:51Z

garden_ai/app/hpc_notebook.py

+
+    script_dir = os.path.dirname(os.path.abspath(__file__))
+    script_dir = os.path.dirname(script_dir)
+    script_dir = os.path.dirname(script_dir)


script_dir = os.path.dirname(script_dir) goes to parent directory. I am locating the definition file here.

I see. This is pretty unintuitive for a reader of this code. Can you either find a different way to express this or add a comment explaining this?

Friendly ping on this comment @JoeyC12

WillEngler · 2024-07-12T16:39:47Z

garden_ai/app/hpc_notebook.py

+        typer.echo("🚧🌱🚧 Failed to start Jupyter Notebook 🚧🌱🚧")
+    except Exception as e:
+        logger.error(f"An error occurred: {e}")
+        typer.echo("🚧🌱🚧 An unexpected error occurred 🚧🌱🚧")


@JoeyC12 Can you address this question?

WillEngler · 2024-07-12T16:58:11Z

garden_ai/app/hpc_notebook.py

+
+    tmp_dir = os.path.join(working_directory, "tmp")
+    if os.path.exists(tmp_dir):
+        shutil.rmtree(tmp_dir)


I think the Python tempfile utils always return different filenames, so I don't think this cleanup step does much in practice

Deleted this line

WillEngler · 2024-07-12T17:04:26Z

garden_ai/app/hpc_notebook.py

+        os.makedirs(notebooks_dir)
+
+    # Step 1: Create temporary directory if it doesn't exist
+    working_directory = tempfile.mkdtemp()


I think it would be least surprising if we make the singularity working directory the user's current working directory. (ie, where they executed garden-ai).

If you want to go with temp dirs instead, we'll need to clean it up properly. (ie, use the TemporaryDirectory context manager)

@JoeyC12 Can you address this?

…e user is at

…when user hit control-c

WillEngler

Thanks for getting these edits in! This looks good to merge

mtilyxuegao and others added 8 commits June 25, 2024 17:25

Set up for issue #492

1d71f1b

Complete hpc-notebook start function

9a11b3a

Complete hpc-notebook start function

b7f5c19

Complete hpc-notebook start function

4bb425c

Complete hpc-notebook start function

6215c40

Complete hpc-notebook start function

5d2c895

Complete hpc-notebook start function

9693872

Complete hpc-notebook start function

e127fa5

WillEngler requested changes Jul 2, 2024

View reviewed changes

mtilyxuegao and others added 7 commits July 3, 2024 12:30

none

51b1d6f

Remove tmp directory. If notebook directory doesn't exit, create one.…

97f19a1

… Move definition file to scripts and use absolute path.

fix

dcc0c2d

Add rerun command

014f4c4

Add rerun command

db06c9b

Add rerun command

97cabd1

Remove allow root

c7543b9

WillEngler requested changes Jul 12, 2024

View reviewed changes

JoeyC12 added 7 commits July 13, 2024 16:25

notebooks dir and the sif file are now created in working dir that th…

9a9f171

…e user is at

notebooks dir and the sif file are now created in working dir that th…

b586b87

…e user is at

Clean up tmp files after process is terminated. Add termination code …

09d63d3

…when user hit control-c

Clean up tmp files after process is terminated. Add termination code …

010a5a1

…when user hit control-c

Changes maade on getting the path to definition file

fe4bf57

Changes maade on getting the path to definition file

4b02464

remove unused variables

c569100

WillEngler approved these changes Jul 24, 2024

View reviewed changes

WillEngler merged commit b6ebc9e into main Jul 24, 2024
6 of 7 checks passed

WillEngler deleted the issue-492 branch July 24, 2024 18:39

WillEngler restored the issue-492 branch July 24, 2024 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#492 #493

#492 #493

JoeyC12 commented Jul 1, 2024 •

edited by github-actions bot

Loading

codecov-commenter commented Jul 1, 2024 •

edited by codecov bot

Loading

WillEngler left a comment

WillEngler Jul 2, 2024

WillEngler Jul 2, 2024

JoeyC12 Jul 13, 2024

WillEngler Jul 2, 2024

JoeyC12 Jul 13, 2024

WillEngler Jul 2, 2024

JoeyC12 Jul 13, 2024

WillEngler Jul 2, 2024

JoeyC12 Jul 13, 2024

WillEngler Jul 2, 2024

JoeyC12 Jul 13, 2024

WillEngler Jul 2, 2024

WillEngler Jul 2, 2024

WillEngler Jul 12, 2024

WillEngler Jul 12, 2024

WillEngler Jul 12, 2024

WillEngler Jul 12, 2024

JoeyC12 Jul 13, 2024

WillEngler Jul 15, 2024

WillEngler Jul 22, 2024

WillEngler Jul 12, 2024

WillEngler Jul 12, 2024

JoeyC12 Jul 13, 2024

WillEngler Jul 12, 2024

WillEngler Jul 15, 2024

WillEngler left a comment


		%startscript

		exec /bin/bash -c "jupyter notebook --notebook-dir=/notebooks --ip 0.0.0.0 --no-browser --allow-root"

#492 #493

#492 #493

Conversation

JoeyC12 commented Jul 1, 2024 • edited by github-actions bot Loading

codecov-commenter commented Jul 1, 2024 • edited by codecov bot Loading

Codecov Report

WillEngler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillEngler left a comment

Choose a reason for hiding this comment

JoeyC12 commented Jul 1, 2024 •

edited by github-actions bot

Loading

codecov-commenter commented Jul 1, 2024 •

edited by codecov bot

Loading