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

Resolve all path in Kedro #3742

Merged
merged 11 commits into from
Apr 2, 2024
Merged

Resolve all path in Kedro #3742

merged 11 commits into from
Apr 2, 2024

Conversation

noklam
Copy link
Contributor

@noklam noklam commented Mar 26, 2024

Description

Related:

Development notes

def _add_src_to_path(source_dir: Path, project_path: Path) -> None:
   ... 

def bootstrap_project(project_path: Path) -> ProjectMetadata:
    """Run setup required at the beginning of the workflow
    when running in project mode, and return project metadata.
    """
    project_path = Path(project_path).resolve() # New
    metadata = _get_project_metadata(project_path)
    _add_src_to_path(metadata.source_dir, project_path)
    configure_project(metadata.package_name)
    return metadata

If you look at the bootstrap_project, it expect a path object but in fact sometimes string is passed and it is okay about this. Same go as _add_src_to_path that accepts Path only but actually taking str sometimes.

The fix here is simple, make sure project_path is resolved. It is already done in many place, just not consistently (sometimes with expanduser() too):

"""
project_path = Path(project_path).expanduser().resolve()

and

source_dir = Path(metadata_dict.get("source_dir", "src")).expanduser()
source_dir = (project_path / source_dir).resolve()
metadata_dict["tools"] = metadata_dict.get("tools")

  • Todo: add a test

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

@noklam
Copy link
Contributor Author

noklam commented Mar 26, 2024

3831e82 this seems to work already

Signed-off-by: Nok Lam Chan <[email protected]>
@noklam noklam marked this pull request as ready for review March 27, 2024 16:17
@noklam noklam requested a review from merelcht as a code owner March 27, 2024 16:17
@noklam noklam requested a review from ankatiyar March 27, 2024 16:17
Signed-off-by: Nok Lam Chan <[email protected]>
@@ -34,7 +36,7 @@ def _version_mismatch_error(kedro_init_version: str) -> str:
)


def _get_project_metadata(project_path: Union[str, Path]) -> ProjectMetadata:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hesitate a bit on whether it should accept both str | Path. As you can see it breaks one of our test.

On the other hand it's much cleaner to keep this path conversion happen at the beginning, i.e. bootstrap_project. Otherwise we are doing the same conversion over and over, and this is the only public function.

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! 👍 It makes total sense to do the path resolution as early on in the flow as possible.

@merelcht merelcht mentioned this pull request Apr 2, 2024
10 tasks
Copy link
Contributor

@ankatiyar ankatiyar 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! Also came across this issue on window tests in kedro-org/kedro-plugins#599.

@noklam noklam merged commit 67a4df0 into main Apr 2, 2024
34 checks passed
@noklam noklam deleted the noklam/symlink branch April 2, 2024 09:40
doxenix pushed a commit to doxenix/kedro that referenced this pull request Apr 3, 2024
* 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]>
AhdraMeraliQB pushed a commit that referenced this pull request Apr 17, 2024
* 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]>
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]>
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.

Symlink resolution during configuration detection might break some CLI commands
3 participants