Skip to content

P/init install cleanup#444

Open
peterhollender wants to merge 7 commits into
mainfrom
p/init_install_cleanup
Open

P/init install cleanup#444
peterhollender wants to merge 7 commits into
mainfrom
p/init_install_cleanup

Conversation

@peterhollender
Copy link
Copy Markdown
Contributor

@peterhollender peterhollender commented Apr 7, 2026

This is a fairly major change to the project, designed to address issues #8 and #10

  • Trims unnecessary parts of the project, including
    • legacy scripts
    • the io module (now covered in openlifu-sdk)
    • unused parts of Database (gridweights)
  • Reorganizes virtual_fit module into seg
  • Eliminates "eager" imports in openlifu.__init__, to prevent the many-seconds-long importing when accessing any submodule.
  • Moves importing of heavier-weight packages (which are being made optional by this branch) into the methods that call them, so that the objects can be partially-used without having those dependencies imported.
  • Introduces sets of optional-dependencies to pyproject.toml that allow for reduced dependency installation. The openlifu-test-app project, for example, can package only the hardware-communication required dependencies by using the [io] option. The base application as used in Slicer should now be installed with the [app] option.

Code that touches openlifu.virtual_fit needs to be refactored to use openlifu.seg.virtualfit, and code that used things like openlifu.Transducer will need to be changed to use openlifu.xdc.Transducer

@peterhollender
Copy link
Copy Markdown
Contributor Author

peterhollender commented Apr 7, 2026 •
edited
Loading

Interesting - the CI virtual environment isn't picking up the right dependencies (because the "no-option" version grabs a super-minimal set that won't have full functionality). Not sure how to address this? I'd really like pip install openlifu to get the primary dependencies with the [app] option, but I'm not sure how to make pyproject.toml do that

@peterhollender peterhollender force-pushed the p/init_install_cleanup branch from faba8a3 to c1e3ae3 Compare April 7, 2026 15:05
@ebrahimebrahim
Copy link
Copy Markdown
Collaborator

ebrahimebrahim commented Apr 7, 2026

Interesting - the CI virtual environment isn't picking up the right dependencies (because the "no-option" version grabs a super-minimal set that won't have full functionality). Not sure how to address this? I'd really like pip install openlifu to get the primary dependencies with the [app] option, but I'm not sure how to make pyproject.toml do that

I think this is not possible, to make pip install openlifu default to pip install openlifu[app]. It will always be the minimal thing.

But that is completely fine, in my opinion. It is very reasonable to make the CI ofr example install pip install openlifu[app] or whatever we want, and to make the primary instructions be pip install openlifu[app] or wahtever. This is common and reasonable.

One way to achieve something like what you are saying is to rename this package to openlifu-core and make a new package called openlifu that thinly wraps it and that depends on openlifu-core[app]. But... eh.

@ebrahimebrahim ebrahimebrahim mentioned this pull request Apr 9, 2026
Merged
ebrahimebrahim
ebrahimebrahim requested changes Apr 10, 2026
View reviewed changes
Comment thread pyproject.toml Outdated
Comment thread pyproject.toml Outdated
Comment thread pyproject.toml Outdated
Comment thread pyproject.toml
Comment thread pyproject.toml Outdated
@sadhana-r sadhana-r mentioned this pull request Apr 28, 2026
Draft
2 tasks
@peterhollender
Group dependencies #10
6f8e12b 
This introduces sets of optional-dependencies to `pyproject.toml` that allow for reduced dependency installation. The `openlifu-test-app` project, for example, can package only the hardware-communication required dependencies by using the `[io]` option. The base application as used in Slicer should now be installed with the `[app]` option.
@peterhollender
Reorganize Project and Imports #8
1605ae7 
- Trims unnecessary parts of the project, including
  - legacy scripts
  - the io module (now covered in `openlifu-sdk`)
  - unused parts of Database (gridweights)
- Reorganizes `virtual_fit` module into `seg`
- Eliminates "eager" imports in `openlifu.__init__`, to prevent the many-seconds-long importing when accessing any submodule.
- Moves importing of heavier-weight packages (which are being made optional by this branch) into the methods that call them, so that the objects can be partially-used without having those dependencies imported.

Code that touches `openlifu.virtual_fit` needs to be refactored to use `openlifu.seg.virtualfit`, and code that used things like `openlifu.Transducer` will need to be changed to use `openlifu.xdc.Transducer`

reorganize virtual fit and geo

remove gridweights

Unused enhancement

Update transducer.py

remove legacy scripts,
@peterhollender
Update noxfile #10
c42cf00
@peterhollender
Fix a few dependency grouping suggestions #10
dc3b039
@peterhollender
Update sdk to 1.0.6 to relax python requirement to 3.10 #10
444569b
@peterhollender peterhollender force-pushed the p/init_install_cleanup branch from e479cbd to 444569b Compare May 19, 2026 13:46
@peterhollender peterhollender force-pushed the p/init_install_cleanup branch from 2f9e682 to 1208ede Compare June 5, 2026 15:26
ebrahimebrahim
ebrahimebrahim requested changes Jun 5, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@ebrahimebrahim ebrahimebrahim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good and we got good results from a hardware enabled test, except one remaining issue: openlifu-sdk doesn't actually support python 3.10!

To see the failure, you can try installing openlifu[io] from this branch in a Python 3.10 environment, and then try this:

import openlifu_sdk

I get

ImportError: cannot import name 'timeout' from 'asyncio'

It's fine in Python 3.12.

Two options here:

  • If we want to ditch Python 3.10 compatbility: Release a new openlifu-sdk version with a corrected Requires-Python. Then update the pinned version here to that, and raise the requires-python here in openlifu
  • If we want to keep Python 3.10: Release a new openlifu-sdk version that replaces asyncio.timeout with a python 3.10 compatible approach. Then update the pinned version here to that.

@peterhollender
Copy link
Copy Markdown
Contributor Author

peterhollender commented Jun 5, 2026

Everything looks good and we got good results from a hardware enabled test, except one remaining issue: openlifu-sdk doesn't actually support python 3.10!

To see the failure, you can try installing openlifu[io] from this branch in a Python 3.10 environment, and then try this:

import openlifu_sdk

I get

ImportError: cannot import name 'timeout' from 'asyncio'

It's fine in Python 3.12.

Two options here:

  • If we want to ditch Python 3.10 compatbility: Release a new openlifu-sdk version with a corrected Requires-Python. Then update the pinned version here to that, and raise the requires-python here in openlifu
  • If we want to keep Python 3.10: Release a new openlifu-sdk version that replaces asyncio.timeout with a python 3.10 compatible approach. Then update the pinned version here to that.

Hmmm that actually looks like it's not even accessed... I've seen copilot chuck dumb imports in while you are typing before. Can you try removing that line and seeing if it passes? Would be easy to patch to the sdk

@ebrahimebrahim
Copy link
Copy Markdown
Collaborator

ebrahimebrahim commented Jun 5, 2026

Removing this line

https://github.com/OpenwaterHealth/openlifu-sdk/blob/7ac69bf618bb98d61f244af06c0f3470804fac69/src/openlifu_sdk/io/LIFUHVController.py#L3

from openlifu-sdk makes openlifu be compatible with python 3.10

Can you guys cut a patch release of openlifu-sdk that removes that line? Then we can update the pinned version here and this should be good to go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ebrahimebrahim ebrahimebrahim ebrahimebrahim requested changes

@dblizinski dblizinski dblizinski approved these changes

@sadhana-r sadhana-r Awaiting requested review from sadhana-r

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@peterhollender @ebrahimebrahim @dblizinski