search

cmd-ntrf / jupyter-lmod

Jupyter plugin that provides a tab for TACC Lmod (https://github.com/TACC/Lmod)
MIT License
28 stars 13 forks source link

Support for Tmod and Lmod #62

Closed mahendrapaipuri closed 4 months ago

mahendrapaipuri commented 1 year ago

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:

Suggestions:

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?

mahendrapaipuri commented 1 year ago

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

cmd-ntrf commented 1 year ago

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

Yes.

cmd-ntrf commented 1 year ago
  • 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: https://github.com/jupyterhub/jupyter-server-proxy/issues/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 commented 1 year ago

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!

mahendrapaipuri commented 1 year ago

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

cmd-ntrf commented 1 year ago

@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 commented 1 year ago

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