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

Support for Tmod and Lmod #62

Merged
merged 4 commits into from
May 16, 2024
Merged

Conversation

mahendrapaipuri
Copy link
Contributor

Hello @cmd-ntrf, so I did some refactoring of code base to change the variable names to generic module instead of lmod. Here are the important points:

  • I have moved typescript contents to root dir to match with extension dir layout proposed in cookie-cutter. This will help in local development as well as we can run jlpm run watch in development mode.
  • Added handlers to get found module system name and logo
  • Changed extension named to @jupyterlab/jupyterlab-lmod to match standard convention. If you prefer to have your namespace @cmd-ntrf, let me know, I will change it back.
  • Distributing yarn.lock is also normally practiced within the community
  • I have tested it on JupyterLab without any issues. But I could not make it work on notebook that uses jupyter_server>=2. Seems like this is due to the way auth is working in jupyter_server>=2 (Ref: PR). When I reverted back to jupyter_server<2, it worked as expected.

Suggestions:

  • What do you think about making MODULE_REGEX and MODULE_HIDDEN_REGEX configurable? They can be sometimes platform dependent.
  • Do you fancy writing some basic UI tests using galata? I never worked with this framework but I would like to give it a try.

We can discuss the changes here and I will add unit tests shortly!! As we are changing the API interface (at least the names), I think this deserves a major release. Maybe we can aim to do release along with JupyterLab 4, which is supposed to be in May. What do you think?

- Typescript contents are moved to root dir to match with extension dir
layout
- Renamed lmod to module
- Added handlers to get found module system name and logo
- Renamed most of the instances of lmod to module except for pkg name
- Added Lmod and Tmod logos to static dir
- Added editable build command to pyproject.toml
- Added npm-run-all to package.json
- Changed extension named to @jupyterlab/jupyterlab-lmod to match
standard convention
- Included yarn.lock with repo
- Added install.json file
@cmd-ntrf cmd-ntrf self-requested a review March 31, 2023 15:56
@cmd-ntrf cmd-ntrf self-assigned this Mar 31, 2023
@mahendrapaipuri mahendrapaipuri marked this pull request as draft April 1, 2023 09:39
@mahendrapaipuri mahendrapaipuri marked this pull request as ready for review April 1, 2023 10:15
@mahendrapaipuri
Copy link
Contributor Author

@cmd-ntrf Maybe it is a good idea to add tests in a separate PR? Just to have a cleaner discussion?

@cmd-ntrf
Copy link
Owner

cmd-ntrf commented Apr 6, 2023

@cmd-ntrf Maybe it is a good idea to add tests in a separate PR? Just to have a cleaner discussion?

Yes.

@cmd-ntrf
Copy link
Owner

cmd-ntrf commented Apr 6, 2023

  • Changed extension named to @jupyterlab/jupyterlab-lmod to match standard convention. If you prefer to have your namespace @cmd-ntrf, let me know, I will change it back.

I think the name of the organization should correspond to an npm org where we can host the package. jupyter-server-proxy recently went through a similar discussion:
jupyterhub/jupyter-server-proxy#363

And to be honest, I would prefer jupyter-lmod to be adopted at some point by some jupyter related org.

  • Distributing yarn.lock is also normally practiced within the community

Agreed.

  • I have tested it on JupyterLab without any issues. But I could not make it work on notebook that uses jupyter_server>=2. Seems like this is due to the way auth is working in jupyter_server>=2 (Ref: PR). When I reverted back to jupyter_server<2, it worked as expected.

With some luck, that issue might be fixed by the time we release the new version of jupyter-lmod.

Suggestions:

  • What do you think about making MODULE_REGEX and MODULE_HIDDEN_REGEX configurable? They can be sometimes platform dependent.

We could and if we do, we would need to document it properly. If you look back in the history of jupyter-lmod, a recurring bug was related to these regexes. Getting them right was not easy, so people should modified them only if they really not what they are doing.

  • Do you fancy writing some basic UI tests using galata? I never worked with this framework but I would like to give it a try.

I have never worked with it either, but I am willing to give it a shot.

We can discuss the changes here and I will add unit tests shortly!! As we are changing the API interface (at least the names), I think this deserves a major release. Maybe we can aim to do release along with JupyterLab 4, which is supposed to be in May. What do you think?

I agree with aiming to do the release along JupyterLab 4. We will also have to think about Notebook 7, which is entirely based on JupyterLab.

@mahendrapaipuri
Copy link
Contributor Author

I think the name of the organization should correspond to an npm org where we can host the package. jupyter-server-proxy recently went through a similar discussion: jupyterhub/jupyter-server-proxy#363

And to be honest, I would prefer jupyter-lmod to be adopted at some point by some jupyter related org.

Fair enough! Have not thought about distributing the npm package.

We could and if we do, we would need to document it properly. If you look back in the history of jupyter-lmod, a recurring bug was related to these regexes. Getting them right was not easy, so people should modified them only if they really not what they are doing.

I mean we can keep the same defaults that are being used now. So, if users do not want to configure, they will get the same behavior. Anyways, configurable regexes can be made in a separate PR.

I agree with aiming to do the release along JupyterLab 4. We will also have to think about Notebook 7, which is entirely based on JupyterLab.

Agree! Havent tested it on Notebook 7 though. Changelog says extensions made for Notebook < 7 would not work on 7. I will test it and see what I can do!

Tmod and Lmod emits slightly difference messages when listing no loaded modules.
Correct module system detection method call in reset.
@mahendrapaipuri
Copy link
Contributor Author

@cmd-ntrf Did you have time to look into these PRs?

@cmd-ntrf
Copy link
Owner

cmd-ntrf commented Jun 2, 2023

@mahendrapaipuri Please accept my deepest apologies, I am currently relocating with my family and I did have not time to work on this. I should be able to give you feedback / merge in a couple weeks.

Again sorry for the late reply.

@mahendrapaipuri
Copy link
Contributor Author

Hey @cmd-ntrf No problem at all. Just checking in!!

@cmd-ntrf cmd-ntrf merged commit e5cb4e7 into cmd-ntrf:main May 16, 2024
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.

2 participants