Skip to content
This repository was archived by the owner on May 21, 2024. It is now read-only.

Reorganize#129

Merged
ysimonson merged 21 commits intomasterfrom
reorg-2
Oct 23, 2019
Merged

Reorganize#129
ysimonson merged 21 commits intomasterfrom
reorg-2

Conversation

@ysimonson
Copy link
Copy Markdown
Contributor

@ysimonson ysimonson commented Sep 20, 2019

A broad reorganization of the library:

  • Renamed python_pachyderm.client has been renamed to python_pachyderm.proto, to more clearly specify what it is.
  • All client code merged into a single class, python_pachyderm.Client.
  • Use python enums to define protobuf enums (rather than a separate variable for each enum variant.)
  • Use of magic to import protos and define python enums. This will better ensure that the library maintains parity with the protos.

This makes the library more closely aligned with the Pachyderm CLI, which doesn't distinguish between the Pachyderm services (PFS vs PPS vs ...) The use of python enums also makes this library a bit more pythonic.

Downsides of this:

  • It's a breaking change, and users will have to make some changes to their code to use this.
  • The Client library is over 1kloc, which is a bit much.

@ysimonson ysimonson changed the title Reorganize Reorganize - Second Variant Sep 20, 2019
@gabrielgrant
Copy link
Copy Markdown

as discussed directly: this layout (that more directly maps to how pachctl lays out functionality, rather than directly reflecting the internal protos) seems somewhat more ergonomic, but also more of a maintenance headache than the approach proposed in #128 . @ysimonson is going to investigate if there is a way to use metaprogramming to reduce the manual maintenance burden (the main question is what is the best way to have a blacklist of objects and props exposed from the raw GRPC compiled output, rather than having to explicitly whitelist everything?)

@ysimonson
Copy link
Copy Markdown
Contributor Author

ysimonson commented Sep 26, 2019

Protos are now imported, and enums are now built, using metaprogramming. A big downside of this approach is that it appears pdoc doesn't recognize the enums as existing, so it doesn't show up in the docs. I may try a hybridized approach, where protos are dynamically imported but a manual mapping of enums is still maintained. Regardless, this line of work seems more promising than #128, so I will close that out.

@ysimonson
Copy link
Copy Markdown
Contributor Author

OK, I managed to get pdoc to document the enums. It's not perfect; it doesn't include the enum variants in the documentation, but at least it's discoverable.

@ysimonson ysimonson changed the title Reorganize - Second Variant Reorganize Sep 26, 2019
@jdoliner
Copy link
Copy Markdown
Member

jdoliner commented Oct 9, 2019

Reading over this I think the idea looks right here. One thing that's not totally clear to me because I haven't written python in a while. How much manual work is required to keep this up to date as new proto changes are added? My sense is that this is a big step forward in that regard compared to where we were before, but not entirely sure.

@ysimonson
Copy link
Copy Markdown
Contributor Author

It is. We're no longer maintaining a mapping of proto names -> python names. The enums mentioned in the PR description are even created with metaprogramming so that they don't need to be manually maintained.

Copy link
Copy Markdown
Member

@jdoliner jdoliner 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 LGTM.

@ysimonson ysimonson merged commit 9b32c9f into master Oct 23, 2019
@ysimonson ysimonson deleted the reorg-2 branch October 23, 2019 20:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants