smroid / cedar-solve

A fast lost-in-space plate solver for star trackers.
https://tetra3.readthedocs.io/en/latest/
Apache License 2.0
3 stars 0 forks source link

pypi wheels and other potential improvements #6

Open leocov-dev opened 3 months ago

leocov-dev commented 3 months ago

@smroid The maintainers over at the main tetra3 project don't seem to be particularly active. how would you feel about bringing this PR adding PyPi wheel publishing over to your branch? This would need a few minor changes to publish under a different package name but I'd be happy to put in the work. My personal opinion is that tetra3 is severely diminished by: not being able to configure the path for generate_database() and not being available on PyPi. Its very hard to use it as a library in a typical python workflow without these 2 changes.

As a bolder step I would propose rebuilding this repo as tetra4 since there are significant improvements here. You'd free yourself from API compatibility without impacting the original project in any way. If ESA ever wanted to pull it under their umbrella you could always donate/transfer the repo to them.


Incremental Plan:

smroid commented 3 months ago

@leocov-dev I'll be happy to accept your PR. I'm not terribly familiar with things like PyPi wheel publishing and Github workflows, so I'll be relying on your expertise.

Regarding the rename cedar-solve to tetra4 idea: I'm still hoping that the tetra3 maintainers will adopt the cedar-solve improvements such that cedar-solve can cease development, with things like PiFinder going back to depending on (the improved) tetra3.

If this doesn't happen, then my preference would be to retain the cedar-solve name for the repo. In addition to the already published cedar-solve and cedar-detect, I'm about to publish a number of other cedar-xxx components. The 'Cedar' name is an umbrella for my efforts around imaging, plate solving, and telescope control for improving enjoyment of visual astronomy.

leocov-dev commented 3 months ago

That sounds good to me, I'll prepare the changes in chunks over the next week so they can be reviewed more easily.

leocov-dev commented 3 months ago

I've updated the description with a basic plan. I'd suggest that it may be good to create a feature branch that PR's can target so that these changes can be reviewed and merged without disturbing the master branch. This will make it easier to check that everything is working as desired and then later can be merged as a whole into master.

leocov-dev commented 3 months ago

reading through your branch I noticed the cedar_detect portions of the code rely on communicating with an external service written in rust (very cool). This is nit ideal for a library since a user would need to think about how to get the service binary. You could make the code assume that the binary is available via the users PATH but a better option would be to build a python wrapper on the cedar_detect features. I have some experience building and releasing python wrappers on rust code and can attempt some of that work over on that repo.

A wrapper on cedar_detect would publish pre-compiled wheels for linux, win, mac such that cedar_detect could be an optional dependency in cedar_solve. This would allow seamless use of the updated centroids function without making cedar_detect a required dependency no configuration would be needed.

A user could then be instructed like so:

# if you want to install only the cedar-solve fork of tetra3
pip install cedar-solve

# if you want to install the cedar-solve fork with the cedar-detect `extract_centroids()` function enabled
pip install cedar-solve[cedar-detect]
smroid commented 3 months ago

I've updated the description with a basic plan. I'd suggest that it may be good to create a feature branch that PR's can target so that these changes can be reviewed and merged without disturbing the master branch. This will make it easier to check that everything is working as desired and then later can be merged as a whole into master.

Sounds good. I've created a PyPi branch.

smroid commented 3 months ago

Sounds great!

reading through your branch I noticed the cedar_detect portions of the code rely on communicating with an external service written in rust (very cool). This is nit ideal for a library since a user would need to think about how to get the service binary. You could make the code assume that the binary is available via the users PATH but a better option would be to build a python wrapper on the cedar_detect features. I have some experience building and releasing python wrappers on rust code and can attempt some of that work over on that repo.

A wrapper on cedar_detect would publish pre-compiled wheels for linux, win, mac such that cedar_detect could be an optional dependency in cedar_solve. This would allow seamless use of the updated centroids function without making cedar_detect a required dependency no configuration would be needed.

A user could then be instructed like so:

# if you want to install only the cedar-solve fork of tetra3
pip install cedar-solve

# if you want to install the cedar-solve fork with the cedar-detect `extract_centroids()` function enabled
pip install cedar-solve[cedar-detect]
AstroKeith commented 2 months ago

Thanks for the work here. A couple of issues ...

  1. in cedar_detect_pb2_grpc.py, line 5 it should be 'from tetra3 import cedar_detect_pb2 as cedardetectpb2'
  2. The pip install doesnt make the needed cedar-detect-server binaries.
AstroKeith commented 2 months ago

Having problems using this pip install. It includes a specific legacy version of Pillow (8.4.0), which fails badly. Tried install Pillow first, no problem it defaulted to 10.4.0 Tried again with pip install cedar-solve, which promptly tried to uninstall 10.4.0 and then fails to install 8.4.0

OS: Debian 12