-
Notifications
You must be signed in to change notification settings - Fork 17
feat: Package as Python library #29
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
feat: Package as Python library #29
Conversation
|
By the way, I highly advise that this become a squash and merge PR to make it a bit easier to manage. Otherwise I'll have to go in and do a bunch of rebasing and squashing to make it a much more manageable number of commits that are all meaningful and not a bunch of dev and testing. |
These exists now as independnet scripts when installed in the virtual envrionment
78a46be
to
92297c7
Compare
@dguest This is ready for review now. I think you'll have some questions and comments, but in advance of all that I'd like to request that when this PR gets merged in that it is squashed and merged with the following commit log (instead of just the full list of all the commits in this PR): Requested squash and merge commit log
|
- master | ||
tags: | ||
- v* | ||
pull_request: | ||
branches: | ||
- master |
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.
Neat, renaming the branches closed this MR. @matthewfeickert I don't know if you can retraget to master, that should be the same branch you'd initially intended to merge to. |
Thanks for reopening this so that it properly targets |
@matthewfeickert, sorry this MR is hanging right now. I'm tempted to wait until we have a conclusion for #24. I guess we can discuss there: if it's going to take ages for CERN to fix something I suppose we should just merge this (or come up with another way to solve the error, i.e. by having instructions to self-sign the CERN root cert). |
Yeah I'm of like mind. Given that @kratsg and I pinged the JIRA ticket we might see some more activity soon. If we don't have a resolution in the next week then we can discuss trying to move forward with some other temporary solution. |
README.md
Outdated
@@ -1,6 +1,7 @@ | |||
Pandamonium | |||
=========== | |||
# Pandamonium |
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.
why you always gotta hate on my underline style? I like the thing that looks more like pretty in raw text form.
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're limited in the level of headings that you can use with this RST-like format. Additionally, it looks very much like the document is mixed RST and Makrdown which is pretty strange. (In all seriousness, have you tried writing in RST before? I'm not sure if you would like it more or less than Markdown).
I'll change this back though.
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.
@dguest Is it possible to go to the equivalent of ###
heading with this RST-like syntax? I didn't rewrite all of the README with this as it isn't clear how I'd do so given this limitation. It seems that this only goes to h2 headers given https://www.markdownguide.org/basic-syntax/#headings
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.
yeah that's true. I always thought it was fine because ###
it emphasizes these points less than a big fat underline. But I shouldn't hold up a MR over this obviously!
README.md
Outdated
### Notes if working on a remote server | ||
|
||
If you are working from a remote server where you do not have control over your Python runtimes (e.g. LXPLUS, ALTAS Connect login nodes) it is recommended that you bootstrap `virtualenv` and a default Python virtual environment by adding the following to your `.profile` or `.bash_profile` | ||
|
||
``` | ||
# Ensure local virtualenv setup | ||
if [ ! -f "${HOME}/opt/venv/bin/virtualenv" ]; then | ||
curl -sL --location --output /tmp/virtualenv.pyz https://bootstrap.pypa.io/virtualenv.pyz | ||
python /tmp/virtualenv.pyz ~/opt/venv # Change this to python3 if available | ||
~/opt/venv/bin/pip install --upgrade pip | ||
~/opt/venv/bin/pip install virtualenv | ||
mkdir -p ~/bin # Ensure exists if new machine | ||
ln -s ~/opt/venv/bin/virtualenv ~/bin/virtualenv | ||
fi | ||
|
||
# default venv from `virtualenv "${HOME}/.venvs/base"` | ||
if [ -d "${HOME}/.venvs/base" ]; then | ||
source "${HOME}/.venvs/base/bin/activate" | ||
fi | ||
``` | ||
|
||
After that source your `.profile` or `.bash_profile` and then if you want to create a default Python virtual environment run | ||
|
||
``` | ||
virtualenv "${HOME}/.venvs/base" | ||
``` | ||
|
||
You will now be dropped into a virtual environment named `base` each time you login. | ||
The virtual environment is not special in anyway, so you should treat it as you would any other. | ||
|
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.
I would put this after the initial instructions. It's a lot of detail that people might already know. It's good to have it earlier but I'd rather keep the executive summary up front.
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.
Moved this around now. So that it comes last under the Installation section.
README.md
Outdated
### Install from PyPI | ||
|
||
You can install [`pandamonium` from PyPI][pandamonium_PyPI] into any Python virtual environment by simply running | ||
|
||
``` | ||
python -m pip install pandamonium | ||
``` | ||
|
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 for example would be perfect higher up.
But I have an issue with your line wrapping. The idea here was that you should be able to run head -n100 README.md
and get the basic instructions. If you're reading in a terminal it's sort of nice to have things wrap at 80.
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.
Fixed this. This is good info to add in a CONTRIBUTING.md
.
README.md
Outdated
|
||
#### Deprecation Warning | ||
|
||
You can currently just clone the repository and have `master` work the same way as `v0.1` on LXPLUS or ATLAS Connect, but this will be deprecated in the future in favor of installing `pandamonium` as a Python library. | ||
The motivation for this is that `pandamonium` does have hard requirements on other libraries, and it is better to fully contain them through the installation of the library through PyPI. | ||
|
||
[pandamonium_PyPI]: https://pypi.org/project/pandamonium/ | ||
|
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 put after "use" section
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.
Done.
In fact I think it might make sense to introduce another section on "job control" or something that talks about the stuff you can do once you've set up EMI: the resub / kill scripts won't work without that. It's probably also useful to make the distinction so people don't think they have to use EMI to run But that doesn't have to go into this MR either. |
@dguest I think I've fixedup the I've additionally made Issues #31 and #32 to get addressed in separate PRs that can be focused on documentation, as this PR is focused on migration to a packaged repo structure. |
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.
Minor comments, but I'll address them after this review.
If you want to install the original version of `pandamonium` before it became a | ||
library and was a set of global level Python scripts you can still do that. | ||
|
||
1. Clone the repository at tag [`v0.1`][tag_v0.1] |
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.
I think this is still out of date. It will work with master as well, won't it?
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.
It is true that it will work with master
but as that could possibly be deprecated in the future I wanted to add instructions that will for sure work.
### Notes if working on a remote server | ||
|
||
If you are working from a remote server where you do not have control over your | ||
Python runtimes (e.g. LXPLUS, ALTAS Connect login nodes) it is recommended that | ||
you bootstrap `virtualenv` and a default Python virtual environment by adding | ||
the following to your `.bashrc` or `.bashrc_user` | ||
|
||
``` | ||
# Ensure local virtualenv setup | ||
if [ ! -f "${HOME}/opt/venv/bin/virtualenv" ]; then | ||
curl -sL --location --output /tmp/virtualenv.pyz https://bootstrap.pypa.io/virtualenv.pyz | ||
python /tmp/virtualenv.pyz ~/opt/venv # Change this to python3 if available | ||
~/opt/venv/bin/pip install --upgrade pip | ||
~/opt/venv/bin/pip install virtualenv | ||
mkdir -p ~/bin # Ensure exists if new machine | ||
ln -s ~/opt/venv/bin/virtualenv ~/bin/virtualenv | ||
fi | ||
|
||
# default venv from `virtualenv "${HOME}/.venvs/base"` | ||
if [ -d "${HOME}/.venvs/base" ]; then | ||
source "${HOME}/.venvs/base/bin/activate" | ||
fi | ||
``` | ||
|
||
After that source your `.profile` or `.bash_profile` and then if you want to | ||
create a default Python virtual environment run | ||
|
||
``` | ||
virtualenv "${HOME}/.venvs/base" | ||
``` | ||
|
||
You will now be dropped into a virtual environment named `base` each time you login. | ||
The virtual environment is not special in anyway, so you should treat it as you | ||
would any other. |
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.
I would put this after "use", and I think it's more appropriately called "working with virtualenv" or something like that. If someone has never used a venv this is a lot to dump on them, and if they have I guess they don't need this information so early on.
This PR packages
pandamonium
as a modern Python library using asrc/
directory structure that is dependent only on the requiredpanda-client
library on which it builds. It adds thepandamonium
scripts as console scripts with the same CLI API and behavior within the scope of the virtual environment that the library is installed into. The old style of keeping things as global level scripts that can be installed and depend on existing globalpanda-client
at the expense of making the library modules executable.Requested squash and merge commit log
It does not use
Click
to prove a CLI API for the time being as there is some discussion with @dguest on the desired level of dependencies. It is better to have the core functionality exist, so that after Issue #27 (providing a tag for the old style of global scope scripts) discussions can be had on this point.A BSD3 license is also added which attributes @dguest as the author and copyright holder.
Additionally, GitHub Action workflows that lint the project with
Black
andpyflakes
are added, in addition to a workflow that ensures that the library can be built usingpep517
.As an example of how this looks at the dev stage on ATLAS Connect (that provides the required authentication certs that I setup below with
emi
and are the focus of Issue #24):Keeping the old style means once can still do this