Local annotations setup and minor dependency handling #750
Conversation
New PaSh version
- Modified `pa.sh` for compatibility with local annotations - Added `local-annotations-library-documentation.md` with setup instructions - Introduced `local-or-pypi.sh` to configure package installation mode for local annotations Signed-off-by: Divik Chotani [email protected] Signed-off-by: Divik Chotani <[email protected]>
There was a problem hiding this comment.
Thanks a lot for the changes Divik :) Here are some comments that you can address before we look at this again. Two more things:
- You should retarget this PR to go to
futureinstead ofmain. - You should add a test of this process, this could be next to the other tests that PaSh runs and then add this test to CI too (check how to add CI tests here:
https://github.com/binpash/pash/blob/main/.github/workflows/tight-loop.yaml)
| @@ -0,0 +1,83 @@ | |||
| # PaSh Setup and Execution Guide | |||
There was a problem hiding this comment.
This file should be moved under docs and we should add a pointer sentence to it from the README :)
| If you haven't already cloned PaSh, do so with: | ||
|
|
||
| ```sh | ||
| git clone https://github.com/binpash/pash.git ~/pash |
There was a problem hiding this comment.
No need to clone it in ~/pash, you can just say
git clone https://github.com/binpash/pash.git
cd pash
|
|
||
|
|
||
|
|
||
| Do not forget to export before using pash: `export PASH_TOP=/home/your-username/pash` |
There was a problem hiding this comment.
this should be changed to the directory that pash is stored, no need to specify it here :)
| To make this persistent across terminal sessions, add it to your shell configuration file: | ||
|
|
||
| ```sh | ||
| echo 'export PASH_TOP=/home/your-username/pash >> ~/.bashrc |
There was a problem hiding this comment.
There is a missing single quote here
| ``` | ||
|
|
||
| This will: | ||
| - Check if `~/annotations` exists. |
There was a problem hiding this comment.
Ideally it should also take an annotation repository as a path (or assume it is a sibling to PaSh but not that it is in the home directory).
local-or-pypi.sh
Outdated
| graphviz | ||
| libdash | ||
| pash-annotations==0.2.2 | ||
| shasta==0.1.0 |
There was a problem hiding this comment.
We don't want to rewrite these here, we better just take them from the requirements file
local-or-pypi.sh
Outdated
| exit 1 | ||
| fi | ||
|
|
||
| echo "✅ Requirements file updated successfully." |
There was a problem hiding this comment.
Maybe this file should instead not change the requirement but rather install the local annotations repo. That makes me wonder whether it should be a part of setup-pash.sh and install local annotations when passed a specific flag, like --local-annotations.
pa.sh
Outdated
| @@ -3,7 +3,10 @@ | |||
| export PASH_TOP=${PASH_TOP:-${BASH_SOURCE%/*}} | |||
| export LD_LIBRARY_PATH="$LD_LIBRARY_PATH:/usr/local/lib/" | |||
| # point to the local downloaded folders | |||
| export PYTHONPATH=$PASH_TOP/venv/lib/python3.12/site-packages:$PYTHONPATH | |||
There was a problem hiding this comment.
Is this necessary or can the use of venv be split in a different PR?
pa.sh
Outdated
| export PYTHONPATH="${PASH_TOP}/python_pkgs/:${PYTHONPATH}" | ||
| export PYTHONPATH=~/annotations:$PYTHONPATH |
There was a problem hiding this comment.
This is too hardcoded. Maybe the default value is next to the pash directory (sibling of PaSh top).
requirements.txt
Outdated
| shasta==0.1.0 | ||
| sh-expand>=0.1.6 | ||
| -e /home/divikchotani.linux/annotations |
- Modified `pa.sh` for compatibility with local annotations - Added `local-annotations-library-documentation.md` with setup instructions - Introduced `setup-local.sh` to configure package installation mode for local annotations Signed-off-by: Divik Chotani <[email protected]>
DivikChotani
force-pushed
the
local-annotations-library
branch
from
af58d6e to
cccca4e
Compare
minor change to readme
| @@ -0,0 +1 @@ | |||
| /home/divikchotani.linux/annotations | |||
There was a problem hiding this comment.
This should probably be git-ignored!
There was a problem hiding this comment.
Or completely removed since it is not used anymore
| @@ -4,6 +4,24 @@ export PASH_TOP=${PASH_TOP:-${BASH_SOURCE%/*}} | |||
| export LD_LIBRARY_PATH="$LD_LIBRARY_PATH:/usr/local/lib/" | |||
| # point to the local downloaded folders | |||
| export PYTHONPATH="${PASH_TOP}/python_pkgs/:${PYTHONPATH}" | |||
| export ANNOTATIONS_PATH=$([ -f "$PASH_TOP/local-annotations-path.txt" ] && tr -d '[:space:]' < "$PASH_TOP/local-annotations-path.txt") | |||
There was a problem hiding this comment.
I see where you are going with this but it is very scary and a very big dependency for PaSh. I think the most appropriate way would be to modify PaSh's installation (scripts/setup_pash.sh) to copy the annotation repo in the right place. I could see two ways to do this:
- Either create a small installation script that replaces the annotation repo where pash looks for dependencies (https://github.com/binpash/pash/blob/future/scripts/setup-pash.sh#L37) and call it there (https://github.com/binpash/pash/blob/future/scripts/setup-pash.sh#L37). Then everytime the user modifies the annotation repo they will call this script and replace their local version of annotations. If we do that we also need to create a script that redownloads the latest normal annotations if needed.
- Properly add a new
--local-annotations-diroption to PaSh (https://github.com/binpash/pash/blob/future/compiler/orchestrator_runtime/pash_init_setup.sh#L27) which could be used to update PYTHONPATH to point first to where the local annotation library is. I think this is the preferred solution if it works :)
There was a problem hiding this comment.
Why do we need the changes in pa.sh now? I think we should delete all of them, I don't see why they are necessary.
|
I left some more suggestions for changes. Once these are done, it would also be great to check that this works. That is why adding a test in CI would be great to guarantee we never break this. To do this, we would add a CI file similar to this: https://github.com/binpash/pash/blob/future/.github/workflows/tight-loop.yaml where we download and install pash (similar to this CI file), and then
I understand that this is a lot of work, so this could become a different PR. Let's try to fix the current issues now and merge this PR and then we can see how to make this work. |
- Implemented `--local-annotations-dir` to allow dynamic switching of annotation. - Updated `pash_init_setup.sh` to adjust `PYTHONPATH` based on user input. - Allowed `cli.py` to recognize and prioritize local annotations when provided. - Ensured clean handling of `requirements.txt`, avoiding unnecessary modifications. Signed-off-by: Divik Chotani [email protected] Signed-off-by: Divik Chotani <[email protected]>
- Resolved merge conflicts between remote and local Signed-off-by: Divik Chotani [email protected] Signed-off-by: Divik Chotani <[email protected]>
|
OS:ubuntu-20.04 |
|
OS:ubuntu-24.04 |
| done | ||
|
|
||
| ## `pash_redir_output` and `pash_redir_all_output` are strictly for logging. | ||
| ## | ||
| ## They do not execute their arguments if there is no debugging. | ||
| if [ "$local_annotations" -eq 1 ]; then | ||
| # Comment out the PyPI-installed `pash-annotations` | ||
| sed -i 's/^pash-annotations.*/#&/' "$PASH_TOP/requirements.txt" |
There was a problem hiding this comment.
Why do we need to do that? At this point is anyone reading the requirements.txt?
|
|
||
| if [ "--local-annotations-dir" == "$item" ]; then | ||
| shift | ||
| export ANNOTATIONS_PATH=$(realpath "$2") |
There was a problem hiding this comment.
I found it surprising that this works with $2 here...
|
|
||
| You can also run the full PaSh test suite: | ||
| ```sh | ||
| $PASH_TOP/evaluation/tests/test_evaluation_scripts.sh |
There was a problem hiding this comment.
This doesn't run the PaSh test suite with the local annotations, why do you mention it then?
| @@ -0,0 +1 @@ | |||
| /home/divikchotani.linux/annotations | |||
There was a problem hiding this comment.
Or completely removed since it is not used anymore
| @@ -4,6 +4,24 @@ export PASH_TOP=${PASH_TOP:-${BASH_SOURCE%/*}} | |||
| export LD_LIBRARY_PATH="$LD_LIBRARY_PATH:/usr/local/lib/" | |||
| # point to the local downloaded folders | |||
| export PYTHONPATH="${PASH_TOP}/python_pkgs/:${PYTHONPATH}" | |||
| export ANNOTATIONS_PATH=$([ -f "$PASH_TOP/local-annotations-path.txt" ] && tr -d '[:space:]' < "$PASH_TOP/local-annotations-path.txt") | |||
There was a problem hiding this comment.
Why do we need the changes in pa.sh now? I think we should delete all of them, I don't see why they are necessary.
| ## **🔹 Step 3: Run setup-annotations.sh to Configure Package Installation** | ||
| PaSh provides a script to configure dependencies, including an option to install local annotations. | ||
|
|
||
| To install dependencies normally: |
| ANNOTATIONS_PATH=$(realpath "$ANNOTATIONS_DIR") | ||
| echo $ANNOTATIONS_PATH > local-annotations-path.txt | ||
| echo "✅ Annotations repository cloned to: $ANNOTATIONS_PATH" | ||
| grep -qxF "$ANNOTATIONS_PATH" requirements.txt || echo -e "\n-e $ANNOTATIONS_PATH" >> requirements.txt |
There was a problem hiding this comment.
Why do we modify requirements here? Why is this useful?
| @@ -215,6 +215,14 @@ def __init__(self, *args, **kwargs): | |||
| help=" output the preprocessed script", | |||
| action="store_true", | |||
| ) | |||
| self.add_argument( | |||
There was a problem hiding this comment.
This is only useful for the help message, other than that the parsing of the argument just happens outside in pash_init_setup.sh so I am not sure whether all of these options (like nargs, const) are useful
pa.shfor compatibility with local annotationslocal-annotations-library-documentation.mdwith setup instructionslocal-or-pypi.shto configure package installation mode for local annotationsSigned-off-by: Divik Chotani [email protected]