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

Feat: parameterize pipeline class in the primary factory method #1176

Merged
merged 4 commits into from Apr 8, 2024

Conversation

z3z1ma
Copy link
Collaborator

@z3z1ma z3z1ma commented Apr 3, 2024

Description

This is an easy addition to parameterize and will simplify some hackery in cdf. Furthermore it makes the factory method significantly more flexible in possibilities since we can pass subclasses of Pipeline.

Related Issues

  • Fixes #...
  • Closes #...
  • Resolves #...

Additional Context

In the future, it would be even simpler if this sort of factory method was a classmethod on the Pipeline class and simply aliased/exported like pipeline = Pipeline.create where Pipeline.create has a sig like def create(cls, ...) which also lets subclasses benefit from the "standard" setup procedure.

For now, we make the simpler change though...

Copy link

netlify bot commented Apr 3, 2024

Deploy Preview for dlt-hub-docs ready!

Name Link
🔨 Latest commit 48cdfa1
🔍 Latest deploy log https://app.netlify.com/sites/dlt-hub-docs/deploys/6612e0f84536560008535dba
😎 Deploy Preview https://deploy-preview-1176--dlt-hub-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

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

for some reason linter is not able to assign types to instances of pipeline created via dlt.pipeline. looks weird. maybe try to temporarily remove the pipeline()` overload (it is not parametrized)

somehow this worked with DltSource

@rudolfix rudolfix force-pushed the feat/parametrize-pipeline-cls branch from a72df2e to 48cdfa1 Compare April 7, 2024 18:07
@rudolfix
Copy link
Collaborator

rudolfix commented Apr 7, 2024

@z3z1ma OK I know what the problem is. there's a special TypeVar in typing extensions that let's you to set a default. I spend several hours looking for this when upgrading DltSoure. See 48cdfa1

is this PR doing what you need in CDF now?

@z3z1ma
Copy link
Collaborator Author

z3z1ma commented Apr 8, 2024

Ah I definitely would not have found that for some time 😅

Thanks for the update @rudolfix 🙌

I just tried it out and indeed it works perfectly. This is very powerful.

@rudolfix rudolfix merged commit 4c6bdbc into devel Apr 8, 2024
43 of 44 checks passed
@rudolfix rudolfix deleted the feat/parametrize-pipeline-cls branch April 8, 2024 10:22
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.

None yet

2 participants