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

Add mlruns to gitignore to avoid pushing mlflow local runs to github #3765

Merged
merged 4 commits into from
Apr 3, 2024

Conversation

Galileo-Galilei
Copy link
Member

@Galileo-Galilei Galileo-Galilei commented Apr 1, 2024

Description

I've noticed an increasing number of beginners facing some issues when using kedro and kedro-mlflow with the following workflow (see : Galileo-Galilei/kedro-mlflow#456):

  • They develop locally and start from a starer or the default template
  • They develop locally and mlflow creates a mlruns folder the first time they run a kedro pipeline
  • They push the local mlflow runs to a remote forge (Github / Gitlab)
  • If someone else pull the code on another machine, mlflow completely messes up artifacts path and they got troubles running their pipelines.

This all boils down to a misunderstanding of how mlflow and git works ; that said they are complex tools and beginners are often introduced to the whole tooling kedro / mlflow / git simultaneously, so it is understandable they do not get the point of not pushing data (their mlflow runs) to a remote forge. Besides, kedro-mlflow (and mlflow itself) do a lot of thing under the hood so some people do not even notice that a folder is created.

I suggest we ignore mlruns/* by default in kedro's template to avoid pushing data remotely, because that's the right thing to do (even if people use mlflow without kedro-mlflow) and that's hard to fix by kedro-mlflow (see Galileo-Galilei/kedro-mlflow#523). I think a beginner will think twice before removing something from .gitignore if they don't really understand what is at stake.

Note: The change should also be done in starters.

Development notes

Add mlruns/* to gitgnore, to avoid pushing mlruns and all its subfolders in case it is created by kedro-mlflow. We may document why this folder should not be pushed, but it is likely to be done in kedro-mlflow, or mlflow iteslef.

Developer Certificate of Origin

We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a Signed-off-by line in the commit message. See our wiki for guidance.

If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.

Checklist

  • Read the contributing guidelines
  • Signed off each commit with a Developer Certificate of Origin (DCO)
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

@Galileo-Galilei Galileo-Galilei force-pushed the add-mlruns-to-gitignore branch from c214c7c to a3e41bd Compare April 1, 2024 13:39
@Galileo-Galilei
Copy link
Member Author

There are some weird doc compiling errors likely totally unrelated to my PR, should I investigae or is this a known issue I can ignore?

@Galileo-Galilei Galileo-Galilei marked this pull request as ready for review April 1, 2024 13:49
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@merelcht merelcht requested a review from noklam April 2, 2024 15:58
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

+1!

To clarify, we'd need to do this change in the starters too, am I right?

Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

Thanks!

@noklam noklam enabled auto-merge (squash) April 3, 2024 14:17
@noklam noklam merged commit caf745b into main Apr 3, 2024
41 checks passed
@noklam noklam deleted the add-mlruns-to-gitignore branch April 3, 2024 14:36
doxenix pushed a commit to doxenix/kedro that referenced this pull request Apr 3, 2024
…edro-org#3765)

* Add mlruns to gitignore to avoid pushing  mlflow local runs to github

Signed-off-by: Yolan Honoré-Rougé <[email protected]>

* update release.md

Signed-off-by: Yolan Honoré-Rougé <[email protected]>

---------

Signed-off-by: Yolan Honoré-Rougé <[email protected]>
Signed-off-by: Merel Theisen <[email protected]>
Co-authored-by: Merel Theisen <[email protected]>
Co-authored-by: Nok Lam Chan <[email protected]>
AhdraMeraliQB pushed a commit that referenced this pull request Apr 17, 2024
…3765)

* Add mlruns to gitignore to avoid pushing  mlflow local runs to github

Signed-off-by: Yolan Honoré-Rougé <[email protected]>

* update release.md

Signed-off-by: Yolan Honoré-Rougé <[email protected]>

---------

Signed-off-by: Yolan Honoré-Rougé <[email protected]>
Signed-off-by: Merel Theisen <[email protected]>
Co-authored-by: Merel Theisen <[email protected]>
Co-authored-by: Nok Lam Chan <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>
AhdraMeraliQB added a commit that referenced this pull request Apr 17, 2024
)

* Update kedro-catalog-0.19.json (#3724)

* Update kedro-catalog-0.19.json

Signed-off-by: Anthony De Bortoli <[email protected]>

* Update set_up_vscode.md

Signed-off-by: Anthony De Bortoli <[email protected]>

---------

Signed-off-by: Anthony De Bortoli <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Update project tests directory structure in docs

Signed-off-by: Ahdra Merali <[email protected]>

* Add docs on writing tests

Signed-off-by: Ahdra Merali <[email protected]>

* Drop dependency on toposort in favour of built-in graphlib (#3728)

* Replace toposort with graphlib (built-in from Python 3.9)

Signed-off-by: Ivan Danov <[email protected]>

* Create toposort groups only when needed

Signed-off-by: Ivan Danov <[email protected]>

* Update RELEASE.md and graphlib version constraints

Signed-off-by: Ivan Danov <[email protected]>

* Remove mypy-toposort

Signed-off-by: Ivan Danov <[email protected]>

* Ensure that the suggest resume test has no node ordering requirement

Signed-off-by: Ivan Danov <[email protected]>

* Ensure stable toposorting by grouping and ungrouping the result

Signed-off-by: Ivan Danov <[email protected]>

---------

Signed-off-by: Ivan Danov <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Optimise pipeline addition and creation (#3730)

* Create toposort groups only when needed
* Ensure that the suggest resume test has no node ordering requirement
* Ensure stable toposorting by grouping and ungrouping the result
* Delay toposorting until pipeline.nodes is used
* Avoid using .nodes when topological order or new copy is unneeded
* Copy the nodes only if tags are provided
* Remove unnecessary condition in self.nodes

Signed-off-by: Ivan Danov <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Expand robots.txt for Kedro-Viz and Kedro-Datasets docs (#3729)

* Add project to robots.txt

Signed-off-by: Dmitry Sorokin <[email protected]>

* Add EOF

Signed-off-by: Dmitry Sorokin <[email protected]>

---------

Signed-off-by: Dmitry Sorokin <[email protected]>
Co-authored-by: Juan Luis Cano Rodríguez <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Kedro need more uv (#3740)

* Kedro need more uv

Signed-off-by: Nok <[email protected]>

* remove docker

Signed-off-by: Nok <[email protected]>

---------

Signed-off-by: Nok <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Resolve all path in Kedro (#3742)

* Kedro need more uv

Signed-off-by: Nok <[email protected]>

* remove docker

Signed-off-by: Nok <[email protected]>

* fix broken type hint and resolve project path

Signed-off-by: Nok Lam Chan <[email protected]>

* fix type hint

Signed-off-by: Nok Lam Chan <[email protected]>

* remove duplicate logic

Signed-off-by: Nok Lam Chan <[email protected]>

* adding nok.py is definitely an accident

Signed-off-by: Nok Lam Chan <[email protected]>

* fix test

Signed-off-by: Nok Lam Chan <[email protected]>

* remove print

Signed-off-by: Nok Lam Chan <[email protected]>

* add test

Signed-off-by: Nok <[email protected]>

---------

Signed-off-by: Nok <[email protected]>
Signed-off-by: Nok Lam Chan <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Remove settings of rate limits and retries (#3769)

* double linkcheck limits

Signed-off-by: Nok Lam Chan <[email protected]>

* fix ratelimit

Signed-off-by: Nok <[email protected]>

---------

Signed-off-by: Nok Lam Chan <[email protected]>
Signed-off-by: Nok <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Improve resume suggestions (#3719)

* Improve suggestions to resume a failed pipeline

- if dataset (or param) is persistent & shared, don't keep looking for ancestors
- only look for ancestors producing impersistent inputs
- minimize number of suggested nodes (= shorter message for the same pipeline)
- testable logic, tests cases outside of scenarios for sequential runner

- Use _EPHEMERAL attribute
- Move tests to separate file
- Docstring updates

---------

Signed-off-by: Ondrej Zacha <[email protected]>
Co-authored-by: Nok Lam Chan <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Build docs fix (#3773)

* Ignored forbidden url

Signed-off-by: Elena Khaustova <[email protected]>

* Returned linkscheck retries

Signed-off-by: Elena Khaustova <[email protected]>

* Removed odd comment

Signed-off-by: Elena Khaustova <[email protected]>

---------

Signed-off-by: Elena Khaustova <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Clarify docs around custom resolvers (#3759)

* Updated custom resolver docs section

Signed-off-by: Elena Khaustova <[email protected]>

* Updated advanced configuration section for consistency

Signed-off-by: Elena Khaustova <[email protected]>

* Updated RELEASE.md

Signed-off-by: Elena Khaustova <[email protected]>

* Updated RELEASE.md

Signed-off-by: Elena Khaustova <[email protected]>

* Test linkcheck_workers decrease

Signed-off-by: Elena Khaustova <[email protected]>

* Increased the By default, the linkcheck_rate_limit_timeout to default

Signed-off-by: Elena Khaustova <[email protected]>

* Returned old docs build settings

Signed-off-by: Elena Khaustova <[email protected]>

* Fixed typo

Signed-off-by: Elena Khaustova <[email protected]>

* Ignore forbidden url

Signed-off-by: Elena Khaustova <[email protected]>

* Returned linkcheck retries

Signed-off-by: Elena Khaustova <[email protected]>

---------

Signed-off-by: Elena Khaustova <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Add mlruns to gitignore to avoid pushing  mlflow local runs to github (#3765)

* Add mlruns to gitignore to avoid pushing  mlflow local runs to github

Signed-off-by: Yolan Honoré-Rougé <[email protected]>

* update release.md

Signed-off-by: Yolan Honoré-Rougé <[email protected]>

---------

Signed-off-by: Yolan Honoré-Rougé <[email protected]>
Signed-off-by: Merel Theisen <[email protected]>
Co-authored-by: Merel Theisen <[email protected]>
Co-authored-by: Nok Lam Chan <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Update the dependencies page in the docs (#3772)

* Update the dependencies page

Signed-off-by: Ankita Katiyar <[email protected]>

* Update docs/source/kedro_project_setup/dependencies.md

Signed-off-by: Ankita Katiyar <[email protected]>

* Apply suggestions from code review

Co-authored-by: Jo Stichbury <[email protected]>
Signed-off-by: Ankita Katiyar <[email protected]>

* Fix lint

Signed-off-by: Ankita Katiyar <[email protected]>

* Move the last line to notes

Signed-off-by: Ankita Katiyar <[email protected]>

---------

Signed-off-by: Ankita Katiyar <[email protected]>
Signed-off-by: Ankita Katiyar <[email protected]>
Co-authored-by: Jo Stichbury <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Change pipeline test location to project root/tests (#3731)

* Change pipeline test location to project root/tests

Signed-off-by: lrcouto <[email protected]>

* Fix some test_pipeline tests

Signed-off-by: lrcouto <[email protected]>

* Change delete pipeline to account for new structure

Signed-off-by: lrcouto <[email protected]>

* Fix some tests

Signed-off-by: lrcouto <[email protected]>

* Change tests path on micropkg

Signed-off-by: lrcouto <[email protected]>

* Fix remaining tests

Signed-off-by: lrcouto <[email protected]>

* Add changes to release notes

Signed-off-by: lrcouto <[email protected]>

* Update file structure on micropackaging doc page

Signed-off-by: lrcouto <[email protected]>

---------

Signed-off-by: lrcouto <[email protected]>
Signed-off-by: L. R. Couto <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Add an option for kedro new to skip telemetry (#3701)

* First draft for telemetry consent flag on kedro new

Signed-off-by: lrcouto <[email protected]>

* Add functioning --telemetry option to kedro new

Signed-off-by: lrcouto <[email protected]>

* Update tests to acknowledge new flag

Signed-off-by: lrcouto <[email protected]>

* Add tests for kedro new --telemetry flag

Signed-off-by: lrcouto <[email protected]>

* Add changes to documentation and release notes

Signed-off-by: lrcouto <[email protected]>

* Minor change to docs

Signed-off-by: lrcouto <[email protected]>

* Lint

Signed-off-by: lrcouto <[email protected]>

* Remove outdated comment and correct type hint

Signed-off-by: lrcouto <[email protected]>

* Update docs/source/get_started/new_project.md

Co-authored-by: Merel Theisen <[email protected]>
Signed-off-by: L. R. Couto <[email protected]>

* Lint

Signed-off-by: lrcouto <[email protected]>

* Minor change on release note

Signed-off-by: lrcouto <[email protected]>

---------

Signed-off-by: lrcouto <[email protected]>
Signed-off-by: L. R. Couto <[email protected]>
Co-authored-by: Merel Theisen <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Update documentation for OmegaConfigLoader (#3778)

* Update documentation for OmegaConfigLoader

Signed-off-by: Puneet Saini <[email protected]>

* Update RELEASE.md

Signed-off-by: Puneet Saini <[email protected]>

* Update RELEASE.md

Signed-off-by: Puneet Saini <[email protected]>

* Update ignore-names.txt

Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>

---------

Signed-off-by: Puneet Saini <[email protected]>
Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>
Co-authored-by: Juan Luis Cano Rodríguez <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Fix path

Signed-off-by: Ahdra Merali <[email protected]>

* Lint

Signed-off-by: Ahdra Merali <[email protected]>

* Add changes to RELEASE.md

Signed-off-by: Ahdra Merali <[email protected]>

* Address comments from code review

Signed-off-by: Ahdra Merali <[email protected]>

* Empty

Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Remove unneeded imports

Signed-off-by: Ahdra Merali <[email protected]>

* Change recommendation from pytest config to editable install

Signed-off-by: Ahdra Merali <[email protected]>

* Add negative testing example

Signed-off-by: Ahdra Merali <[email protected]>

* Replace Dict with dict

Signed-off-by: Ahdra Merali <[email protected]>

* Remove test classes

Signed-off-by: Ahdra Merali <[email protected]>

* Change the assert step for the integration test

Signed-off-by: Ahdra Merali <[email protected]>

* Fix error handling for OmegaConfigLoader (#3784)

* Update omegaconf_config.py

Signed-off-by: Puneet Saini <[email protected]>

* Update RELEASE.md

Signed-off-by: Puneet Saini <[email protected]>

* add a more complicated test case

Signed-off-by: Nok <[email protected]>

---------

Signed-off-by: Puneet Saini <[email protected]>
Signed-off-by: Nok <[email protected]>
Co-authored-by: Nok <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Add Simon Brugman to TSC (#3780)

Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Update technical_steering_committee.md (#3796)

Signed-off-by: Marcin Zabłocki <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Remove jmespath dependency (#3797)

Signed-off-by: Merel Theisen <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Update spaceflights tutorial and starter requirements for kedro-datasets optional dependencies (#3664)

* Update spaceflights tutorial and starter requirements

Signed-off-by: lrcouto <[email protected]>

* fix e2e tests

Signed-off-by: lrcouto <[email protected]>

* Fix e2e tests by distinguishing `kedro-datasets` dependency for different python versions (#3802)

Signed-off-by: Merel Theisen <[email protected]>

* Update docs/source/tutorial/tutorial_template.md

Co-authored-by: Merel Theisen <[email protected]>
Signed-off-by: L. R. Couto <[email protected]>

---------

Signed-off-by: lrcouto <[email protected]>
Signed-off-by: L. R. Couto <[email protected]>
Signed-off-by: Merel Theisen <[email protected]>
Co-authored-by: Merel Theisen <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Consider Vale's suggestions

Signed-off-by: Ahdra Merali <[email protected]>

* Hide test in details

Signed-off-by: Ahdra Merali <[email protected]>

* Quick fix

Signed-off-by: Ahdra Merali <[email protected]>

* Typo (and wording changes)

Signed-off-by: Ahdra Merali <[email protected]>

* Update robots.txt (#3803)

Signed-off-by: Ankita Katiyar <[email protected]>
Co-authored-by: Juan Luis Cano Rodríguez <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Add a test for transcoding loops of 1 or more nodes (#3810)

Signed-off-by: Ivan Danov <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Ensure no nodes can depend on themselves even when transcoding is used (#3812)

* Factor out transcoding helpers into a private module

Signed-off-by: Ivan Danov <[email protected]>

* Ensure node input/output validation doesn't allow transcoded self-loops

Signed-off-by: Ivan Danov <[email protected]>

* Updated release note to avoid github warning

Signed-off-by: Elena Khaustova <[email protected]>

---------

Signed-off-by: Ivan Danov <[email protected]>
Signed-off-by: Elena Khaustova <[email protected]>
Co-authored-by: Elena Khaustova <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Update UUID telemetry docs (#3805)

Signed-off-by: Dmitry Sorokin <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Change path to starters test (#3816)

Signed-off-by: lrcouto <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Move changes in RELEASE.md to docs section

Signed-off-by: Ahdra Merali <[email protected]>

* Change formatting

Signed-off-by: Ahdra Merali <[email protected]>

* Revert "Change formatting"

This reverts commit 9582a22.

Signed-off-by: Ahdra Merali <[email protected]>

* Apply changes from code review

Signed-off-by: Ahdra Merali <[email protected]>

* Add explanation on why cleanup isn't needed

Signed-off-by: Ahdra Merali <[email protected]>

* Change assert on successful pipeline to check logs

Signed-off-by: Ahdra Merali <[email protected]>

* Update description of integration test under pipeline slicing

Signed-off-by: Ahdra Merali <[email protected]>

* Missing formatting

Signed-off-by: Ahdra Merali <[email protected]>

* Update tests directory structure

Signed-off-by: Ahdra Merali <[email protected]>

---------

Signed-off-by: Anthony De Bortoli <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>
Signed-off-by: Ivan Danov <[email protected]>
Signed-off-by: Dmitry Sorokin <[email protected]>
Signed-off-by: Nok <[email protected]>
Signed-off-by: Nok Lam Chan <[email protected]>
Signed-off-by: Ondrej Zacha <[email protected]>
Signed-off-by: Elena Khaustova <[email protected]>
Signed-off-by: Yolan Honoré-Rougé <[email protected]>
Signed-off-by: Merel Theisen <[email protected]>
Signed-off-by: Ankita Katiyar <[email protected]>
Signed-off-by: Ankita Katiyar <[email protected]>
Signed-off-by: lrcouto <[email protected]>
Signed-off-by: L. R. Couto <[email protected]>
Signed-off-by: Puneet Saini <[email protected]>
Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>
Signed-off-by: Marcin Zabłocki <[email protected]>
Signed-off-by: Merel Theisen <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>
Co-authored-by: Anthony De Bortoli <[email protected]>
Co-authored-by: Ivan Danov <[email protected]>
Co-authored-by: Dmitry Sorokin <[email protected]>
Co-authored-by: Juan Luis Cano Rodríguez <[email protected]>
Co-authored-by: Nok Lam Chan <[email protected]>
Co-authored-by: Ondrej Zacha <[email protected]>
Co-authored-by: ElenaKhaustova <[email protected]>
Co-authored-by: Yolan Honoré-Rougé <[email protected]>
Co-authored-by: Merel Theisen <[email protected]>
Co-authored-by: Ankita Katiyar <[email protected]>
Co-authored-by: Jo Stichbury <[email protected]>
Co-authored-by: L. R. Couto <[email protected]>
Co-authored-by: Puneet Saini <[email protected]>
Co-authored-by: Marcin Zabłocki <[email protected]>
Co-authored-by: Elena Khaustova <[email protected]>
@astrojuanlu astrojuanlu mentioned this pull request Jun 6, 2024
7 tasks
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.

4 participants