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.

Sign up for GitHub

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

Jump to bottom

Package python scripts #75

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Package python scripts #75

wants to merge 13 commits into from

Conversation

SarahAlidoost
Copy link
Member

closes #71

@SarahAlidoost SarahAlidoost marked this pull request as ready for review July 4, 2024 12:16
@SarahAlidoost SarahAlidoost requested a review from fnattino July 4, 2024 12:17
@SarahAlidoost
Copy link
Member Author

I noticed some problems with paths in spider_container_deploy.sh, see issue #76

@fnattino fnattino mentioned this pull request Jul 4, 2024
Open
fnattino
fnattino requested changes Jul 4, 2024
View reviewed changes
Copy link
Contributor

@fnattino fnattino left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks a lot @SarahAlidoost , this already looks very nice! I have left a couple of comments on the pyproject.toml, I think we should aim to have the python package installed with a CLI to access the functionalities of the tool. This PR already brings it quite close to this goal, since it already installs dependencies automatically.

However, to make it fully working directly from the installed CLI, we need to fix a couple of things: there seems to be a circular import between the two Python files, and, most importantly, we need to figure out a global absolute path where to store the config files. I think these aspects might involve some small refactoring of the code, so I would address these in a new PR.

src/installJDOnSLURM.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
src/runJupyterDaskOnSLURM.py Outdated
import installJDOnSLURM #Functions to install or uninstall JDOnSLURM

config_path = './config/platforms/platforms.ini'
# get the parent pat of the current file
parent_path = Path(__file__).resolve().parent
Copy link
Contributor

Choose a reason for hiding this comment

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

By adding one level to the dir above, I guess this would need to be updated?

Copy link
Member Author

Choose a reason for hiding this comment

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

I replaced it with config_path = Path.home() / ".config/platforms/platforms.ini" and in the user-guide.md, I asked users to create it.

user-guide.md Outdated

```shell
python runJupyterDaskOnSLURM.py --add_platform
python src/runJupyterDaskOnSLURM.py --add_platform
Copy link
Contributor

Choose a reason for hiding this comment

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

Also here, one more level should be added.

Copy link
Member Author

Choose a reason for hiding this comment

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

the command python src/runJupyterDaskOnSLURM.py is now replaced by jupyter-dask-on-slurm.

@SarahAlidoost
Copy link
Member Author

SarahAlidoost commented Jul 5, 2024 •
edited
Loading

Thanks a lot @SarahAlidoost , this already looks very nice! I have left a couple of comments on the pyproject.toml, I think we should aim to have the python package installed with a CLI to access the functionalities of the tool. This PR already brings it quite close to this goal, since it already installs dependencies automatically.

However, to make it fully working directly from the installed CLI, we need to fix a couple of things: there seems to be a circular import between the two Python files, and, most importantly, we need to figure out a global absolute path where to store the config files. I think these aspects might involve some small refactoring of the code, so I would address these in a new PR.

@fnattino Thanks a lot, very helpful suggestions. I addressed your comments and also fixed the circular dependencies. Here is a summary of the changes:

  • the function ssh_remote_executor is moved to utils.py to fix the circular dependencies.
  • the config_path is changed to Path.home() / ".config/platforms/platforms.ini" to be able to run the command jupyter-dask-on-slurm independent of WPD. However, the function copy_folder also uses a relative path to copy the repo folder from local to remote. Still, users need to clone the repository and change the working directory, see Issue with copy_folder and install_JD #79.
  • the installation command is now pip install -U git+https://github.com/RS-DAT/JupyterDaskOnSLURM.git which uses code in the main branch by default.
  • the wget -P command is used in user-guide.md to get config files and create directories.
  • The user-guide.md is updated accordingly.

To test the installation in this branch, do pip install -U git+https://github.com/RS-DAT/JupyterDaskOnSLURM.git@fix_71. I tested manually by running the commands for spider. It works fine except for uninstall mode due to the bug reported in issue #77 .

I found the documentation of Configuring dCache is not clear. So I submitted issue #78. Please let me know if this PR needs further improvements.

@fnattino fnattino mentioned this pull request Jul 11, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fnattino fnattino fnattino requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Package Python script
2 participants
@SarahAlidoost @fnattino