Overhaul pythonfinder #135

matteius · 2023-04-29T21:27:58Z

Overhaul pythonfinder to use pydantic and eliminate the WindowsFinder code paths; remove attrs library.

Eliminating WindowsFinder and seeing all the tests pass for Windows in pipenv is a positive thing because not only does it drop the vendor requirement here, it also gets all the operating systems on the same code paths, so it should be easier to maintain going forward.

… code paths; remove attrs library.

…er into overhaul-to-pydantic

matteius · 2023-04-29T22:49:09Z

tests/test_python.py

        assert isinstance(parsed.version, Version)


-@pytest.mark.skip_nt
-def test_shims_are_kept(monkeypatch, no_pyenv_root_envvar, setup_pythons, no_virtual_env):


These two tests were really bad -- they use importlib to rejigger the import of the pydantic classes (reload them) but that has the side effect of re-registering the same validator method again with pydantic which raises a pydantic error. Further, these tests were asserting a lot of things about the mocks it had setup -- I feel they are not adequate tests and were already being skipped on windows for some reason.

matteius · 2023-04-29T23:03:53Z

@finswimmer If you have some time to check out this as it pertains to: pypa/pipenv#5670

finswimmer · 2023-04-30T07:40:07Z

Hey @matteius,

I'm not sure if it was intended to ping me here 😄 I don't have much knowledge about the internals of pythonfinder.

Overall I like the idea to switch to pydantic. (Actually this was something I have in mind as well when I first took some closer look at the codebase) 👍

What I saw so far is, that you are using type comments instead of type annotations. Because pythonfinder supports python >=3.7 only there is no need for type comments anymore. (See #130).

Also you should run pre-commit locally. This is not in the CI pipeline until now, because flake8 is throwing some errors, where I don't know how to fix them.

oz123 · 2023-04-30T08:16:46Z

What I saw so far is, that you are using type comments instead of type annotations. Because pythonfinder supports python >=3.7 only there is no need for type comments anymore. (See #130).

I believe these are just leftovers from the past. One can update these, but this should not be a blocker now.

oz123 · 2023-04-30T08:20:20Z

src/pythonfinder/models/mixins.py


    def __str__(self) -> str:
-        return fs_str(f"{self.path.as_posix()}")
+        return fs_str("{0}".format(self.path.as_posix()))


I believe fs_str can be removed, as well as compat.py. We these all over in vistir and reqlib.
Past issues with py2py3 version and unicode are now now history.

oz123 · 2023-04-30T08:21:58Z

src/pythonfinder/models/mixins.py

-        arch: str | None = None,
-        name: str | None = None,
-    ) -> list[PathEntry]:
+        major=None,  # type: Optional[Union[str, int]]


Is there a reason you used type comments and not type hints directly?

This PR came from I made and verified first in pipenv and then differed in from pipenv -- it seems master pythonfinder was ahead of what was in pipenv, but only in terms of type annotations. I corrected many of them, but I couldn't get to all of them.

The other thing to note about some of the annotations is they weren't validating under all code paths because sometimes the types used in either a test or one of the paths is slightly different, so I had to loosen their types using Any for the time being. I figure there will be more iterating on this in the future, especially once pydantic 2 hits a stable release.

oz123 · 2023-04-30T08:24:59Z

src/pythonfinder/models/path.py

    get_shim_paths,
 )
-from ..exceptions import InvalidPythonVersion
 from ..utils import (
    dedup,


Not an action item here, but I am writing this so we'll remember.
utils.py here contains common stuff with pipenv.
So maybe when we vendor this in pipenv we can remove it from pipenv.utils.
Also, dedup was dropped in favour of one liner.

matteius · 2023-04-30T14:44:29Z

Thanks @finswimmer and @oz123 I addressed the PR feedback and Chat GPT helped me address the type hint method signatures.

matteius · 2023-04-30T14:48:36Z

Also @finswimmer I did mean to tag you because of your recent work here -- this was my first time analyzing and updating the pythonfinder code base -- it took quite some time, but not as much as requirementslib is taking.

finswimmer · 2023-05-02T09:51:40Z

src/pythonfinder/models/common.py

+
+
+class FinderBaseModel(BaseModel):
+    def __setattr__(self, name, value):  # noqa: C901 (ignore complexity)


Hey @matteius,

could you explain to me why a custom implementation is needed for __setattr__ and how this implementation is different from the default behavior?

Thanks a lot!

fin swimmer

pydantic has limitations about private variables (those prefixed with _) that they cannot be set/gotten or or returned in the dict -- I definitely needed it at one point in the refactor, it may be worth experimenting with if its still needed, or will be needed in pydantic 2.0, but I recall running into some initial errors that it got me past at that time.

Thanks for your quick response 👍

Not sure when you first implement this pattern. But I guess this is no longer needed, when setting underscore_attrs_are_private to True (https://docs.pydantic.dev/latest/usage/models/#private-model-attributes), right?

You're welcome to see if the tests pass in pipenv and this library by refactoring this work further -- I just wanted to get my feet wet in this library and have the larger goal of converting pipenv to a more modern framework.

TBBle · 2024-03-04T18:02:12Z

So just looking at using this library in another project, and it seems this commit removed PEP 514 support, but didn't update the docs to show that.

Python installs visible in py --list but not in the system PATH are not find by Pythonfinder 2.0.0 or later, but were visible with Pythonfinder <2.

oz123 · 2024-03-04T18:53:56Z

So just looking at using this library in another project, and it seems this commit removed PEP 514 support, but didn't update the docs to show that.

Python installs visible in py --list but not in the system PATH are not find by Pythonfinder 2.0.0 or later, but were visible with Pythonfinder <2.

Can you specify your OS and how you installed python?

matteius · 2024-03-04T22:53:43Z

@TBBle It was a feature (unknown to me at that time) of the windows library that was vendored in and providing the pythonfinder compatibility there, but it had other bugs; we wanted to unify the code base so that all OS flow through the same code path; there are some open issues in pipenv about this breaking as well and we are open to PRs that would directly add support for py command here in pythonfinder without using that other library for windows that we dropped.

TBBle · 2024-03-05T13:19:59Z

So just looking at using this library in another project, and it seems this commit removed PEP 514 support, but didn't update the docs to show that.
Python installs visible in py --list but not in the system PATH are not find by Pythonfinder 2.0.0 or later, but were visible with Pythonfinder <2.

Can you specify your OS and how you installed python?

I can open a full bug report later if you want, but trivially, this is Windows 11, with Python 3.9 and 3.10 installed as system installs using the official installer, and not added to the PATH. (Unless someone puts up a PR soon, I'd suggest removing PEP 514 from the feature list, as specifically PEP 514 support is what brought me to this tool.) (Edit: #158)

But the actual change I'm talking about is pretty clear, you can look at this PR and see that the PEP 514 library was removed, along with the only caller of PythonVersion.from_windows_launcher, and no registry access code nor library was added.

I guess that PEP 514 support had no unit test coverage, even though it was (and still is) listed on the features page, so this removal slipped under the radar. The current code-base still has no calls to PythonVersion.from_windows_launcher in it, from which I assume the feature wasn't intended to be removed.

I recommend not trying to wrap the py binary, as:

it'll be slower than directly implementing PEP 514, i.e. Windows registry access, as you're shelling out to a binary that is doing exactly that, as of Python 3.11's py.
py doesn't provide a machine-parsable output format, or any guarantee of output format stability.
The whole point of PEP 514 is to allow other tools to populate and access that same information without needing to rely on py even being installed, let alone shell out to it and parse its human-targeted output.

Overhaul pythonfinder to use pydantic and eliminate the WindowsFinder…

5ef4c59

… code paths; remove attrs library.

matteius requested a review from oz123 April 29, 2023 21:28

matteius added 23 commits April 29, 2023 17:29

add missing file

e54d043

Corrected imports

f4b7ee7

addressing build error

11fcf43

address import issue with tests

011575f

address import issue with tests

c5eb2f6

Merge branch 'master' into overhaul-to-pydantic

095c972

Try casting to Path in test

784839c

Merge branch 'overhaul-to-pydantic' of github.com:sarugaku/pythonfind…

62ea0b7

…er into overhaul-to-pydantic

Try casting to PythonFinder in test

7e30a14

Try casting to PythonFinder in test

86f1550

Try casting to PythonFinder in test

93145f8

Try casting to PythonFinder in test

270738f

remove not necceessary validator that may be causing test failures.

e54050e

Refactor imports based on pydantic bug report.

b375139

apply simiilar fix to this test

1fa5706

remove nonexistent method.

2a60b74

Remove unused class

db37936

iterate on last two tests

7d1872f

factor tests away from importlib

c86624e

Assertion was checking that it mocked itself.

3621712

Assertion was checking that it mocked itself.

7292280

Assertion was checking that it mocked itself.

0220167

Remove confusing and bad test cases

8feed67

matteius commented Apr 29, 2023

View reviewed changes

oz123 requested changes Apr 30, 2023

View reviewed changes

matteius added 3 commits April 30, 2023 10:30

PR feedback

fb3c727

PR feedback

ff866d3

PR feedback

6598298

matteius requested a review from oz123 April 30, 2023 14:43

oz123 approved these changes May 1, 2023

View reviewed changes

oz123 merged commit 642642a into master May 1, 2023

finswimmer reviewed May 2, 2023

View reviewed changes

TBBle mentioned this pull request Mar 4, 2024

poetry env use X.Y fails on Windows python-poetry/poetry#2117

Open

3 tasks

TBBle mentioned this pull request Apr 1, 2024

PEP-514 support was removed in version 2.0 but is still documented #158

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul pythonfinder #135

Overhaul pythonfinder #135

matteius commented Apr 29, 2023

matteius Apr 29, 2023

matteius commented Apr 29, 2023

finswimmer commented Apr 30, 2023

oz123 commented Apr 30, 2023

oz123 Apr 30, 2023

oz123 Apr 30, 2023

matteius Apr 30, 2023

matteius Apr 30, 2023

oz123 Apr 30, 2023

matteius commented Apr 30, 2023

matteius commented Apr 30, 2023

finswimmer May 2, 2023

matteius May 2, 2023

finswimmer May 3, 2023

matteius May 3, 2023

TBBle commented Mar 4, 2024

oz123 commented Mar 4, 2024

matteius commented Mar 4, 2024

TBBle commented Mar 5, 2024 •

edited

Loading



		class FinderBaseModel(BaseModel):
		def __setattr__(self, name, value): # noqa: C901 (ignore complexity)

Overhaul pythonfinder #135

Overhaul pythonfinder #135

Conversation

matteius commented Apr 29, 2023

Choose a reason for hiding this comment

matteius commented Apr 29, 2023

finswimmer commented Apr 30, 2023

oz123 commented Apr 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matteius commented Apr 30, 2023

matteius commented Apr 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TBBle commented Mar 4, 2024

oz123 commented Mar 4, 2024

matteius commented Mar 4, 2024

TBBle commented Mar 5, 2024 • edited Loading

TBBle commented Mar 5, 2024 •

edited

Loading