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 .absolute() with .resolve() #2912

Closed
noklam opened this issue Aug 8, 2023 · 11 comments
Closed

Replace .absolute() with .resolve() #2912

noklam opened this issue Aug 8, 2023 · 11 comments
Labels
Issue: Feature Request New feature or improvement to existing feature

Comments

@noklam
Copy link
Contributor

noklam commented Aug 8, 2023

Description

Potentially helps

Context

See #2819 (comment)
See docs:

Path.resolve(strict=False)
Make the path absolute, resolving any symlinks. A new path object is returned:
Python Discussion Thread: https://discuss.python.org/t/pathlib-absolute-vs-resolve/2573/11

image

The only way to remove .. in a path is using .resolve(), this is useful when user need to instantiate DataCatalog in a notebook, because the project root is in ../ one level up.

Possible Implementation

Possible Alternatives

@noklam noklam added the Issue: Feature Request New feature or improvement to existing feature label Aug 8, 2023
@astrojuanlu
Copy link
Member

About symlinks, notice that in #2346 I was advocating for the opposite: replacing .resolve() with .absolute().

this is useful when user need to instantiate DataCatalog in a notebook, because the project root is in ../ one level up.

If we do away with the magic path conversion as proposed in #2965, is this still needed?

@noklam
Copy link
Contributor Author

noklam commented Aug 29, 2023

In [5]: p
Out[5]: PosixPath('../x/ee')

In [6]: p.absolute()
Out[6]: PosixPath('/Users/Nok_Lam_Chan/tmp/pp/../x/ee')

In [7]: p.resolve()
Out[7]: PosixPath('/Users/Nok_Lam_Chan/tmp/x/ee')

I don't think we should replace resolve with absolute

#2977 requires a way to handle hidden objects, the relative path are determined with .. We can use a more sophisticated regex to avoid ../ maybe, but I don't think we should take this approach.

If you want to walk an arbitrary filesystem path upwards, it is recommended to first call Path.resolve() so as to resolve symlinks and eliminate ".." components.

Quote from the docs, shouldn't we want to support symlink? The #2346 is a rather niche case to instantiate KedroCLI from a special directory

Path.absolute()
Make the path absolute, without normalization or resolving symlinks. Returns a new path object:

Path.resolve(strict=False)
Make the path absolute, resolving any symlinks. A new path object is returned:

@noklam
Copy link
Contributor Author

noklam commented Aug 30, 2023

Actually I have a second look. This is the only case that in our codebase use absolute()

def get_pkg_version(reqs_path: (str | Path), package_name: str) -> str:
"""Get package version from requirements.txt.
Args:
reqs_path: Path to requirements.txt file.
package_name: Package to search for.
Returns:
Package and its version as specified in requirements.txt.
Raises:
KedroCliError: If the file specified in ``reqs_path`` does not exist
or ``package_name`` was not found in that file.
"""
reqs_path = Path(reqs_path).absolute()

This is the only leftover we used .absolute() in the codebase.

#2819 (comment)
Additionally, we may always do a resolve in ConfigLoader so even if user provide a relative path it would work. I will add this comment to #2971, depends on the implementation we may avoid this completely with the new root argument.

This ticket will remains as a clean up ticket.

@merelcht
Copy link
Member

Do we still need to do this?

@astrojuanlu
Copy link
Member

I actually think we should close this and do the opposite: #2346

@noklam noklam mentioned this issue Mar 26, 2024
8 tasks
@merelcht
Copy link
Member

merelcht commented Apr 2, 2024

@noklam is this also completed by #3742 ?

@noklam
Copy link
Contributor Author

noklam commented Apr 2, 2024

@merelcht It didn't change by #3742.

def get_pkg_version(reqs_path: (str | Path), package_name: str) -> str:
"""Get package version from requirements.txt.
Args:
reqs_path: Path to requirements.txt file.
package_name: Package to search for.
Returns:
Package and its version as specified in requirements.txt.
Raises:
KedroCliError: If the file specified in ``reqs_path`` does not exist
or ``package_name`` was not found in that file.
"""
reqs_path = Path(reqs_path).absolute()

I just check again, this function is not used anywhere in framework, but only tests. (last change is 4 years ago). Maybe at some points it was used for kedro build-reqs? I suggest we deprecate this function since it's a public function and move it to test.

@astrojuanlu
Copy link
Member

+1

@astrojuanlu
Copy link
Member

@noklam should we rename and re-scope this issue? Alternatively we can close this one as "won't fix" and open a new one about deprecating get_pkg_version

@noklam
Copy link
Contributor Author

noklam commented Apr 8, 2024

@astrojuanlu let's close this ticket and open a new one, the context aren't that relevant here.

I will do it shortly.

@noklam
Copy link
Contributor Author

noklam commented Apr 8, 2024

Closed and open #3789

@astrojuanlu astrojuanlu closed this as not planned Won't fix, can't repro, duplicate, stale Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Feature Request New feature or improvement to existing feature
Projects
Archived in project
Development

No branches or pull requests

3 participants