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

Overhaul Python common package build #421

Conversation

marcelamelara
Copy link
Member

@marcelamelara marcelamelara commented Mar 29, 2023

This PR overhauls how we build and install the python library:

  • Switches the build for the SWIG Python bindings to use the updated cmake build infra
  • Consolidates client and core python packages
  • Builds a Python wheel for the common/client and sservice
    - [ ] Write pdo.common.crypto wrapper
  • Documents updated python package structure

@cmickeyb
Copy link
Contributor

This looks like a great start! I'll have to spend more time with the CMakefile to understand all the ins & outs of what you're doing there.

One other note... should we consider packaging the SWIG files separately? that is, move them out of the python directory tree into one of their own and keep the python tree strictly python? (and maybe combine it with the client tree)

@cmickeyb
Copy link
Contributor

One thing I would really like to see from this... can we get to where we can build the packages without the virtual environment in place? Clearly we need the dependencies to run, but do we need them to build? This is part of pushing for a clean separation between the build, install, configure and run phases.

@marcelamelara
Copy link
Member Author

can we get to where we can build the packages without the virtual environment in place? Clearly we need the dependencies to run, but do we need them to build?

I'll look into this, because agreed, it would be nice to separate out the different phases more cleanly.

"viewBackgroundColor": "#ffffff"
},
"files": {}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a particular reason to include this? have you checked license implications?

Copy link
Member Author

Choose a reason for hiding this comment

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

This provides the definitions for the figures. In case they need to be edited later down the line, anyone should be able to open them up in an Excalidraw editor. As far as I can tell, the editor is licensed under MIT, and their terms say that content created in their editor belongs to the creator. Happy to remove these files.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is Excalidraw a common tool? I've never heard of it. Any reason not to make them something more commonly used (google draw?). Or maybe i'm just way behind the times.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think excalidraw is actually being used under the hood by other whiteboard applications, I just realized recently that there's a separate in-browser service as well.

@cmickeyb
Copy link
Contributor

On a related note... #433 moves the directory structure of the ledgers to align with the value in PDO_LEDGER_TYPE. Could we set up the python packages to load from a common directory in each of the ledgers (e.g. PDO_SOURCE_ROOT/ledgers/ccf/python) to pull in modules for submitter, install, and maybe key generation? (And maybe the tools as well.) This might make it possible to remove all ledger-specific code from the base python packages.

@marcelamelara
Copy link
Member Author

marcelamelara commented Apr 14, 2023

This might make it possible to remove all ledger-specific code from the base python packages.

All components use the ledger APIs, though, don't they? I'd rather keep as much of the python code in one place as possible. It makes it a bit easier to track, IMO, and also makes the python package build more straight-forward. That said, I am in favor of mirroring the ledgers/ccf directory structure within python/pdo.

@cmickeyb
Copy link
Contributor

cmickeyb commented Apr 14, 2023

This might make it possible to remove all ledger-specific code from the base python packages.

All components use the ledger APIs, though, don't they? I'd rather keep as much of the python code in one place as possible. It makes it a bit easier to track, IMO, and also makes the python package build more straight-forward. That said, I am in favor of mirroring the ledgers/ccf directory structure within python/pdo.

The only ledger specific code we have right now is the submitter and a few of the key functions. Those can be pulled into the package (setup includes the designated directory in the ledger/$PDO_LEDGER_TYPE/python directory. There is a super class for the submitter that stays in python...

OK... i guess there is more... the registration and configuration functions.

Point is... I would like to keep all the CCF related stuff together.
And we can require at least the client install for CCF.

@marcelamelara
Copy link
Member Author

Maybe I'm missing something, but why do we want to build and configure CCF separately? I understand doing this for the TP, though the submitter is used by several other components besides the client.

I can move the CCF-specific submitter to the ccf directory since it's just one file, but in general, I would like to avoid/minimize having more python code spread across a bunch of different directories. Right now we have python code mixed with cpp code in practically every directory, and I think the dependencies will be clearer, and also easier to maintain, if we separate the cpp layers from the python layers.

@cmickeyb
Copy link
Contributor

cmickeyb commented Apr 14, 2023

Maybe I'm missing something, but why do we want to build and configure CCF separately? I understand doing this for the TP, though the submitter is used by several other components besides the client.

I can move the CCF-specific submitter to the ccf directory since it's just one file, but in general, I would like to avoid/minimize having more python code spread across a bunch of different directories. Right now we have python code mixed with cpp code in practically every directory, and I think the dependencies will be clearer, and also easier to maintain, if we separate the cpp layers from the python layers.

i don't want to build it separately. I want the python to pull the packages from PDO_SOURCE_ROOT/ledgers/ccf/python instead. That way all of the ccf code is in one place rather than scattered throughout PDO_SOURCE_ROOT/python.

and in the end it isn't just one file... we already reach into the ccf directory for the registration code. and we have several places where we customize for ccf keys. and... the configuration PR has a chunk of code that is used to configure ccf.

And... this can probably wait for a separate PR.

@cmickeyb
Copy link
Contributor

Right now we have python code mixed with cpp code in practically every directory, and I think the dependencies will be clearer, and also easier to maintain, if we separate the cpp layers from the python layers.

This is precisely because we have separate packages. We do NOT want the eservice or pservice code being packaged with the client package because it would pull in all the sgx dependencies. We should be building something like:

  • eservice package
  • pservice package
  • sservice package
  • client package
  • service support package

we may also want a "ledger" package. but i'm ok rolling this into the client/service support packages

@marcelamelara
Copy link
Member Author

We do NOT want the eservice or pservice code being packaged with the client package because it would pull in all the sgx dependencies.

I 100% agree AND this can be accomplished even if all of the python code is in a single directory.

@marcelamelara marcelamelara force-pushed the python-install-overhaul branch 2 times, most recently from 41d7ea6 to dee1115 Compare April 18, 2023 16:49
@marcelamelara
Copy link
Member Author

@cmickeyb We currently ship the contents of $PDO_HOME/opt/pdo/bin and $PDO_HOME/opt/pdo/etc with pdo.common (see python/setup.py data_files parameter to setup). I don't think we need either of those anymore? Especially the contents of /etc, which include ledger keys, seem rather problematic to distribute as part of pdo.common...

@marcelamelara marcelamelara force-pushed the python-install-overhaul branch 2 times, most recently from c51f2f2 to a687bdb Compare April 18, 2023 23:59
@marcelamelara marcelamelara changed the title Overhaul Python package build and install Overhaul Python package build Apr 24, 2023
@marcelamelara marcelamelara force-pushed the python-install-overhaul branch 6 times, most recently from 3116179 to 05a5ae6 Compare April 26, 2023 17:50
@marcelamelara marcelamelara marked this pull request as ready for review April 26, 2023 17:51
@marcelamelara marcelamelara changed the title Overhaul Python package build Overhaul Python common package build Apr 26, 2023
@marcelamelara marcelamelara marked this pull request as draft April 26, 2023 19:51
@cmickeyb
Copy link
Contributor

@cmickeyb We currently ship the contents of $PDO_HOME/opt/pdo/bin and $PDO_HOME/opt/pdo/etc with pdo.common (see python/setup.py data_files parameter to setup). I don't think we need either of those anymore? Especially the contents of /etc, which include ledger keys, seem rather problematic to distribute as part of pdo.common...

I think we can drop the etc & bin directories. Nothing goes in them right now.

@marcelamelara marcelamelara added client Issues related to building, configuring and running a client build labels Apr 27, 2023
@marcelamelara marcelamelara force-pushed the python-install-overhaul branch 6 times, most recently from 99d5aa9 to 1244770 Compare May 3, 2023 00:43
@marcelamelara marcelamelara marked this pull request as ready for review May 3, 2023 00:43
@marcelamelara marcelamelara force-pushed the python-install-overhaul branch 2 times, most recently from 951afdd to 11226fb Compare May 3, 2023 15:50
@marcelamelara marcelamelara marked this pull request as draft May 4, 2023 22:18
@marcelamelara marcelamelara force-pushed the python-install-overhaul branch 4 times, most recently from 7cbcae0 to 4c4eee7 Compare June 12, 2023 22:19
* Add figure for package structure
* Add PDO contract registration and invocation figures to docs

Signed-off-by: Marcela Melara <[email protected]>
* Separate SWIG definitions from python common
* Use cmake to build SWIG python bindings

Signed-off-by: Marcela Melara <[email protected]>
* Build PDO common python wheel using pre-generated SWIG bindings
* Update sservice python build to use cmake and wheel dist
* Fix python unit tests

Signed-off-by: Marcela Melara <[email protected]>
* Only include client crypto in pdo common library
* Ensure client is always built with services

Signed-off-by: Marcela Melara <[email protected]>
@marcelamelara
Copy link
Member Author

Closing this to break up into multiple PRs.

@marcelamelara marcelamelara deleted the python-install-overhaul branch March 22, 2024 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build client Issues related to building, configuring and running a client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants