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

DM-37791: use pybind wrappers, merge into one pybind shared lib #306

Merged
merged 1 commit into from Feb 11, 2023

Conversation

mwittgen
Copy link
Contributor

@mwittgen mwittgen commented Feb 3, 2023

No description provided.

Copy link
Contributor

@erykoff erykoff left a comment

Choose a reason for hiding this comment

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

This all looks good. I did think that all the addInheritanceDependency() calls were supposed to be moved to _algorithmsLib.cc?

@erykoff
Copy link
Contributor

erykoff commented Feb 9, 2023

Also, what was the final decision on stub files to allow for easier backwards compatibility? Or was that about meas_base...

@mwittgen
Copy link
Contributor Author

This all looks good. I did think that all the addInheritanceDependency() calls were supposed to be moved to _algorithmsLib.cc?

Indeed, will fix.

@mwittgen
Copy link
Contributor Author

Also, what was the final decision on stub files to allow for easier backwards compatibility? Or was that about meas_base...

Checking with @ktlim and @timj to make sure. For meas_base and meas_algorithm there were not many direct imports of shlibs in other packages, so I didn't add the stub. There was no concern about old config files used with newer pipeline versions.

@timj
Copy link
Member

timj commented Feb 10, 2023

Also, what was the final decision on stub files

We discussed this and we decided that the config case was a red herring since we already have configs that can only be read by the software that wrote them (although I would like @mwittgen to put fixing the excessive import problem on his todo list). We also noted that if anything is doing from .x import * in the __init__.py that the x.py is not really part of the public API even though it could be used like that. We therefore decided it was okay to drop the .py files.

@mwittgen mwittgen merged commit 592df81 into main Feb 11, 2023
@mwittgen mwittgen deleted the tickets/DM-37791 branch February 11, 2023 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants