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

Missing project path prevents from running a project with kedro-airflow #54

Closed
mwiewior opened this issue Aug 24, 2020 · 3 comments · Fixed by #60
Closed

Missing project path prevents from running a project with kedro-airflow #54

mwiewior opened this issue Aug 24, 2020 · 3 comments · Fixed by #60
Labels
need-design-decision Several ways of implementation are possible and one must be chosen
Milestone

Comments

@mwiewior
Copy link

I propose to add project_path to run.py template in #53

        MlflowPipelineHook(
            model_name="{{ cookiecutter.python_package }}", conda_env="{{ cookiecutter.project_path }}/src/requirements.txt",
        ),
@Galileo-Galilei
Copy link
Owner

Hello,

I am aware of this problem and kedro-airflow specifies that paths need to be absolute. There is always a dilemma between keeping path relative (to make the project work "as is" when pulled) or making path absolute (for more declarative configuration and enhanced consistency).

I would incline to accept this, but I want to deprecate model_name and conda_env as arguments of MlflowPipelineHook in a future release because it is much more consistent to pass them as arguments for pipeline_ml rather than of a Hook and mainly because of #29. If we do this, this argument will no longer be auto-filled and you will have to specify this path manually when creating a PipelineML object so it will be up to you to decide whether you want a relative or absolute path.

We have to discuss it with @kaemo, but #29 is a pretty big change and it may not be completed until next release. We may merge your PR even if we expect it to be deprecated on the (short/medium/long?) run.

Let me know if it makes sense to you.

@Galileo-Galilei
Copy link
Owner

Juste for the record @mwiewior, I am curious for why this is actually needed:

  • If you use pipeline_ml, this makes sense but this class is not compatible at all with kedro-airflow plugin (we could give a try to make it more compatible, but is due to airflow not accepting hooks so it will be very hacky and even impossible without collaboration from the kedro team). Just changing the path from relative to absolute will not fix your problem
  • If you do not use pipeline_ml, this argument was not used so I don't understand what kind of failure you faced. Could you give more detailed information and a reproducible example / error message?

@Galileo-Galilei Galileo-Galilei added need-design-decision Several ways of implementation are possible and one must be chosen question labels Sep 29, 2020
@Galileo-Galilei Galileo-Galilei added this to the Release 0.3.0 milestone Sep 29, 2020
@Galileo-Galilei Galileo-Galilei linked a pull request Oct 1, 2020 that will close this issue
2 tasks
@Galileo-Galilei
Copy link
Owner

Galileo-Galilei commented Oct 10, 2020

I close the issue which is fixed in the upcoming release. Feel free to reopen if you have extra questions / informations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need-design-decision Several ways of implementation are possible and one must be chosen
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants