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

Issue with copy_folder and install_JD #79

Open
SarahAlidoost opened this issue Jul 5, 2024 · 2 comments
Open

Issue with copy_folder and install_JD #79

SarahAlidoost opened this issue Jul 5, 2024 · 2 comments

Comments

@SarahAlidoost
Copy link
Contributor

Currently, in the function install_JD:

if not folder_exists:
        print ('Cloning JupyterDaskonSLURM on remote host...')
        utils.ssh_remote_executor(config_inputs, copy_folder, config_inputs)

while the function copy_folder copies a folder instead of cloning. So, the print statement needs a fix. Also, in the function copy_folder, a relative path ../JupyterDaskOnSLURM is used which means first cloning the repo and cd to the directory. Why the whole folder JupyterDaskOnSLURM on remote is needed? can we just copy the scripts that are needed?

@fnattino
Copy link
Contributor

Good point @SarahAlidoost . I believe that the reason why the repository is currently copied to the remote (and not cloned) is that in this way you can modify the scripts locally before uploading them to the platform. But this allows you to modify these scripts only when installing, you will be bound to use these scripts for all time you will be running jupyter..

Maybe a better way to do this could be:

  • when you install jupyter-dask-on-slurm locally, you create a local folder (.config/jupyterdask-on-slurm ?) where you put all the job scripts (jupyter_dask_spider.bsh, jupyter_dask_snellius.bsh, etc.) and platform configurations file (platforms.ini)
  • whenever you run jupyter-dask-on-slurm to start jupyter on the remote, the correct job file is copied to the platform to a temporary folder (/tmp ?) and submitted. Credentials to access the platform would also be in a standard location, so we don't need to track where the platform.ini file is. This way you can update the job script locally and this will be immediately reflected when starting jupyter.
  • Not sure what is the best way to handle the other configuration files (for fsspec, Dask, etc.). Also for these it would be ideal to be able to change some of the fields at run time (say, for this run, you want to increase the number of cores per job, or change the queue for the dask workers). The best way I can think of right now would be to use environment variables - both Dask and fsspec support this way of defining configs, apart from the files.

What do you think? When I am back I could try to implement these changes and open a PR towards fix_71, so that #75 will then solve all these at once.

@fnattino
Copy link
Contributor

Hi @SarahAlidoost and @rogerkuou sorry but I could not get that far in what I indended to do on updating the script to work as I was trying to describe in the previous message. The discussion with @rogerkuou made me think that maybe we should not modify too much the way in which Dask configuration is provided, i.e. via config files, so maybe we should not implement passing parameters via environment variables (see third point above).

I still think we should avoid having to clone the repository on the remote - maybe we could even drop the installation part from the Python script? I have the impression we have been the only ones using it in the courses, while the other people that have been using RS-DAT have preferred to go through the manual installation.. Also, seeing how things have evolved for Spider, maybe we should just give more flexibility on which environment should be used and avoid pushing the use of conda. See also #82.

Maybe worth it to discuss it briefly together once we are all back?

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

No branches or pull requests

2 participants