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

Replace Pipeline with pipeline across all tests #1833

Closed
jmholzer opened this issue Sep 6, 2022 · 8 comments · Fixed by #2189
Closed

Replace Pipeline with pipeline across all tests #1833

jmholzer opened this issue Sep 6, 2022 · 8 comments · Fixed by #2189
Assignees

Comments

@jmholzer
Copy link
Contributor

jmholzer commented Sep 6, 2022

Description

Currently, Pipeline is used to instantiate objects of this class across the testing code base. This should be updated to use the pipeline factory in kedro.pipeline.modular_pipeline.

Context

This comes from @AntonyMilneQB's comment on #1795 suggesting that these changes be made for the test files modified for that feature.

@jmholzer jmholzer added the Issue: Feature Request New feature or improvement to existing feature label Sep 6, 2022
@jmholzer jmholzer removed the Issue: Feature Request New feature or improvement to existing feature label Sep 6, 2022
@ziyad00
Copy link

ziyad00 commented Sep 17, 2022

This is my first issue and I'd like to be assigned

@jmholzer
Copy link
Contributor Author

Excellent! So happy to have you onboard @ziyad00

@adamfrly
Copy link
Contributor

adamfrly commented Jan 9, 2023

Is this issue still open? I'd be happy to pick up the work for it

@antonymilne
Copy link
Contributor

I believe it is indeed still open. @jmholzer?

@jmholzer
Copy link
Contributor Author

Hey @adamfrly, yes this issue is still open 🙂 Awesome! Would be glad to have your contribution.

@antonymilne
Copy link
Contributor

From what I recall I think all the remaining instances of Pipeline cannot be fixed just by find and replace (all the ones that could have already been) since there would be a clash of variable name and function name pipeline. Hence there would be a need to rename some variables here, not just do a naive find and replace. Does that sound right @jmholzer?

@jmholzer
Copy link
Contributor Author

@AntonyMilneQB yes! This is exactly right 🙂. As soon as we import pipeline there will be a good deal of shadow naming going on in many of the files, so variable names will have to be changed.

@adamfrly
Copy link
Contributor

Sounds great! I was thinking of something along the lines of from kedro.pipeline.modular_pipeline import pipeline as modular_pipeline to avoid the clash

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

Successfully merging a pull request may close this issue.

6 participants