-
Notifications
You must be signed in to change notification settings - Fork 25
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
Jupyter notebook integration with TorQ processes #43
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.
So two main high level points:
- I think the main thing that needs to be considered here, is this change adds python as a dependency, unless it's already used somewhere else (maybe @jonnypress can comment). So if we want this to be usable the docs will need updated to say that python is required for this feature (possibly including which version), and you'll need something to help users install the right packages. There's a few ways to do that in python, but probably the easiest here is a requirements.txt file, which you can make using
pip freeze
( I see you mentioned that above anyway) .ipynb
files contain both code, and output cells. When you're committing stuff I don't think it makes sense to include the output cells, as they will just be overwritten as soon as someone runs the notebook, the results are specific to your environment and that point in time, and it massively blows up the size of the notebook. Probably better take those out of your notebook file.
As far as I can see, this uses the unofficial qpython package rather than either PyQ or embedPy/JupyterQ, which I think are the two "official" kx-supported interfaces (the later allowing notebook code to be written in q rather than python, which might be more "user friendly" for q devs). Is there a reason for this choice? |
I'm not really sure if either of those (embedPy/PyQ) are appropriate here, much as I love a bit of PyQ. While you're right, qpython doesn't have the kx blesssing, I think it's quite a bit more battle tested than either of the others. |
jupyternb/jupyterrun.sh
Outdated
|
||
#must source setenv.sh from TORQHOME as setenv uses dirpath | ||
cd .. | ||
source setenv.sh |
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.
Running setenv.sh
before this script should be a requirement and written into the docs. Don't want to be assuming the location of this file.
jupyternb/jupyterrun.sh
Outdated
JUPYTEREMAIL="[email protected]" | ||
|
||
#set variable for jupyter-nbconvert command path | ||
JUPYTERLOC=/home/$USER/.local/bin/jupyter-nbconvert |
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.
Users homedir may be elsewhere, if that is the intention then go for this:
JUPYTERLOC=/home/$USER/.local/bin/jupyter-nbconvert | |
JUPYTERLOC=$HOME/.local/bin/jupyter-nbconvert |
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 line is bad idea in general, there's no guarantee at all that this binary will be in .local
. I think this comes under the same point I raised above, for this whole feature we need some better dependency management to make sure all the stuff that is required is already in place and properly source/on $PATH etc.
echo \"New Jupyter Notebook Generated\" | mail -A ${JUPYTERHTML} -s \"New Jupyter Notebook Generated\" ${JUPYTEREMAIL} | ||
|
||
#Remove most recent notebook, so previous notebooks are not sent | ||
rm ${JUPYTERHTML}" > ${JNBCRONSCRIPT} |
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.
Not sure about storing this inside another script, perhaps this text can be stored in a separate template file instead? Would make it easier to modify too.
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.
Would you prefer a .txt file with the following in it, that is called in this jupyterrun.sh script
\"New Jupyter Notebook Generated\" | mail -A ${JUPYTERHTML} -s \"New Jupyter Notebook Generated\" ${JUPYTEREMAIL}
#Remove most recent notebook, so previous notebooks are not sent
rm ${JUPYTERHTML}"
or to have the JNBCRONSCRIPT as a separate script, already within the jupyternb directory, that will be bashed from this jupyterrun.sh script?
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.
You can have a text file with variables in it properly escaped so that when you echo the file the variables are populated.
Added check Python3 is already installed Updated module installation for Jupyter Notebook Added mailtuils needs to be installed
Integration of Jupyter notebooks to run checks on TorQ processes.
This jnbcronjob.sh file will first convert notebook to html, send an email with the HTML version and then delete the HTML file
It will also create the cronjob to bash this script every 5 minutes
[ ] Need to add dependency.txt file