-
Notifications
You must be signed in to change notification settings - Fork 18
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
SwanProjects extension #252
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't review the ts code yet (either way, you know that way better than anyone else). But in the meantime, I'm adding some comments/nitpicks.
In general, I would add a bit more comments, as it would make things more understandable (even if we can understand after spending some time).
At the end, I just remembered that some of this logic still lives in the contents manager, no? Meaning that there's code shared among both, so maybe we should re-use the utils there, for example.
Or we no longer need it?
@@ -0,0 +1,9 @@ | |||
[bumpversion] | |||
current_version = 0.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be 0.0.0
?
|
||
include swanprojects/kernelmanager/resources/* | ||
include swanprojects/stacks/*/* | ||
include swanprojects/kernels/*/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not include stacks or kernels (maybe the kernels are ok...). To me those are specific and should be treated as config, i.e being part of the docker image and not the extension.
## Install & Configure | ||
|
||
- Install the package with pip | ||
- Configure `SWAN_DEFAULT_ENV_FILE` for the default environment for folders without an environment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would configure a stacks folder, where we put all these file, and we can have a default
one (that we populate from cvmfs)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(sorry, even myself had to go back to try to understand what I mean with this comment)
What I mean is, instead of setting this as an extra variable, we just push the file we generate on startup on the "default" folder.
|
||
- Install the package with pip | ||
- Configure `SWAN_DEFAULT_ENV_FILE` for the default environment for folders without an environment | ||
- Configure `c.SwanProjectsConfig,stacks_path` in you jupyter server configuration to configuration of available software stacks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Configure `c.SwanProjectsConfig,stacks_path` in you jupyter server configuration to configuration of available software stacks | |
- Configure `c.SwanProjectsConfig.stacks_path` in you jupyter server configuration to configuration of available software stacks |
@@ -0,0 +1,6 @@ | |||
#!/bin/bash | |||
# This script allows to source FCCSW stack for a given release and platform. | |||
# variables $RELEASE and $PLATFORM have to be exported before source this script by swan_env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sourcing? particular?
if not setup_script_path.is_file(): | ||
stacks_folder = str(stacks_folder / stack["type"]) | ||
raise Exception(f"Unknown software stack type.") | ||
pass_through_env = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason we do this? Can you document?
f"RETURNCODE: {process.returncode}, STDOUT:\n {stdout}\n, STDERR: \nf{stderr}\n" | ||
) | ||
raise Exception( | ||
f"Error generating environment with output {stdout}, {stderr}, {process.returncode}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is sent to the users, right? Maybe not worth to put all this detail.
"content": user_script, | ||
"format": "text", | ||
} | ||
self.contents_manager.save(model, user_script_file_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you use this method? There're other, lower level ones, that you can use and don't require you to set all of this model stuff.
(same applies to other places you use it)
name, value = line.rstrip("\n").split("=", 1) | ||
kwargs["env"][name] = value | ||
|
||
self.kernel_spec.argv[0] = shutil.which( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document
raise Exception("This kernel is not avaialble in this enviornment.") | ||
return await super().pre_launch(**kwargs) | ||
|
||
def _get_project_root(self, path: str) -> Optional[Path]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the duplication?
@jupyterlab/launcher
to display a dialog to edit environment options for a folderNeeds swan-cern/systemuser-image#81
Based on #209