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

Normal pip install of pixl CLI results in export directory being wrongly determined #318

Open
1 task
jeremyestein opened this issue Feb 22, 2024 · 2 comments
Labels
dev-velocity Working on the issue will increase development speed in the long run

Comments

@jeremyestein
Copy link
Contributor

jeremyestein commented Feb 22, 2024

Definition of Done / Acceptance Criteria

  • The system test should pass whether the pixl CLI packages are installed with or without the -e option to pip install

Details and Comments

See main.yml workflow file:

  system-test:
    steps:
      - name: Install Python dependencies
        # The -e option is required here due to the way the
        # code determines the export directory. See issue #318.
        run: |
          pip install -e pixl_core/
          pip install -e cli/[test]
          pip install -e pytest-pixl/

If the -e options are removed, the system test will fail.

For reference, the root docker-compose.yml:

services:
  ehr-api:
    volumes:
      - ${PWD}/exports:/run/exports

This defines the export directory as in the root of the source code working tree on the GAE, as is intended.

The problem comes with how this directory is determined in the CLI code (cli/src/pixl_cli/_io.py):

HOST_EXPORT_ROOT_DIR = Path(__file__).parents[3] / "exports"

If you have done a "normal" install (pip install cli/), this location will be somewhere in your python libs directory, NOT in your source tree as expected. It's only when installed in editable mode, which is intended for dev use only, that these directories match up.

I guess we can keep installing with -e for now, it just seems kind of broken if your packages can't be installed in the standard way?

The most obvious way I see to fix it is to make it a config option, so it can be passed to docker compose so it can be mounted correctly and the CLI can pick up on it.

@jstutters
Copy link
Contributor

See here for a way to determine the location of within-package files that should work whether or not a package is installed as editable: https://github.com/UCLH-Foundry/PIXL/blob/027f794c90046ef7f1ed3c3a5be75619b992ebbf/pytest-pixl/src/pytest_pixl/dicom.py#L36

@milanmlft milanmlft added the dev-velocity Working on the issue will increase development speed in the long run label Mar 6, 2024
@jeremyestein
Copy link
Contributor Author

See here for a way to determine the location of within-package files that should work whether or not a package is installed as editable:

https://github.com/UCLH-Foundry/PIXL/blob/027f794c90046ef7f1ed3c3a5be75619b992ebbf/pytest-pixl/src/pytest_pixl/dicom.py#L36

It will determine the location correctly (as does __file__ I think?), but that still isn't the right thing to do.
Example of editable vs non-editable installs, for reference:

>>> importlib.resources.files("importlib")
PosixPath('/Users/jeremystein/miniconda3/envs/pixl/lib/python3.10/importlib')
>>> importlib.resources.files("pytest_pixl")
PosixPath('/Users/jeremystein/PIXL/PIXL/pytest-pixl/src/pytest_pixl')

We always want to define the exports dir relative to the source directory, regardless of install type of our packages.

(I don't like putting application data into the source tree, but that's a separate discussion)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev-velocity Working on the issue will increase development speed in the long run
Projects
None yet
Development

No branches or pull requests

4 participants