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

[WIP] Complex Pipelines #144

Closed
wants to merge 1 commit into from
Closed

[WIP] Complex Pipelines #144

wants to merge 1 commit into from

Conversation

ryanhiebert
Copy link
Contributor

References #103

I haven't done any end-to-end testing of this just yet, I've only done some manual spot checking of generated messages to see if they look like what I intend them to look like. I'm not totally sure how this work can or should be integrated into the project, and most of the names I don't really care about, so I'm flexible to rename entities.

This nominally supports arbitrary nesting of three types: Single, Chain, and Group. Single is a wrapper for messages. Chain and Group are for sequential and parallel sets of tasks, respectively. The tasks in a chain or group can be other chains or groups, and conceptually all mixes of them are supported.

However, I haven't yet handled the case of empty groups or empty chains. I expect that empty groups will be something worth having good behavior for, and not cause errors, while empty chains are, IMO, a useless idea and I don't see any reason to support them.

I've created several abstract base classes to help organize my thoughts as I was designing these interfaces. I'm not tied to keeping all those ABCs separate, necessarily, but they were helpful for communicating which functionality is for what purpose.

I'm not tied to the names of all these classes or method names, but I've worked hard on the general design, and I'm pleased with where it's ended up for the most part.

This doesn't currently support passing data to the next task in a chain. My primary objective was to get the flow working, and I believe that it can be addressed after the flow implementation is something that can be reasonable added later. It will involve some extra complexity, because that will also require figuring out how to store and retrieve results for groups. Therefore, I figure it's best to get feedback on the design before I muddy the water trying to bring to many things together at the same time.

One possible improvement, depending on what thoughts we might have for integrating this with the project deeply, would be to allow chains and groups to take messages directly, rather than requiring them to be wrapped in a Single. Leaving the extra wrapping as a requirement was a shortcut to hopefully make this initial review more manageable.

I've written this all with Python 3.6+ in mind, with type annotations. Currently the project support Python 3.5. I'd love to use dataclasses for this work, if we think that could be reasonable, but that would require dropping 3.5 support. What is your plan as far as supporting old versions of Python 3? I don't expect it, but if we wanted to go to Python 3.7-only, I could even make the type signatures a bit neater with a future import. (PEP 563)

@Bogdanp
Copy link
Owner

Bogdanp commented Dec 23, 2018

Thanks for your work on this, but I'm going to close this PR in favor of the approach in #150 because I would like to avoid changing the composition primitives too much.

@Bogdanp Bogdanp closed this Dec 23, 2018
@ryanhiebert
Copy link
Contributor Author

FTR, my intent with this pull request wasn't to merge as-is, or even close to as-is. It was to provide a starting point for discussion of a design that was capable of all the composition primitive combinations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants