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

feature: Fall back to dynamic analysis when static fails #15

Open
nardi opened this issue Dec 2, 2024 · 6 comments
Open

feature: Fall back to dynamic analysis when static fails #15

nardi opened this issue Dec 2, 2024 · 6 comments
Assignees
Labels
feature New feature or request

Comments

@nardi
Copy link

nardi commented Dec 2, 2024

We have some Pydantic models that have dynamically generated field attributes, such as description and default. Currently, the static analyzer crashes on this (when attempting ast.literal_eval), but looking at the code it looks like the dynamic analyzer should be able to handle this. However, I don't see any way to trigger it.

Ideally, when something like this is encountered, the static analyzer would mark the class/attribute in question and additionally perform dynamic analysis on it. If that is not possible/desirable, having a way to manually force dynamic analysis (also when using MkDocs), possibly for specific classes, would be useful as well.

I could work on a PR but first wanted to check if a feature like this was maybe already planned, or if it is purposefully not going to be implemented.

@nardi nardi added the feature New feature or request label Dec 2, 2024
@pawamoy
Copy link
Member

pawamoy commented Dec 2, 2024

Hey @nardi, thanks for the request.

There's this how-to that could be of interest to you: https://mkdocstrings.github.io/griffe/guide/users/how-to/selectively-inspect/. It will show you how to selectively "inspect" (use dynamic analysis on) some objects. In this case, you'd inspect these dynamically generated fields you write about. Let me know if it helps, or if anything is unclear in the how-to.

I'm also interested in getting more info on this ast.literal_eval crash! Could you open a new issue with the traceback and all? That'd be great 🙂

@nardi
Copy link
Author

nardi commented Dec 2, 2024

There's this how-to that could be of interest to you: https://mkdocstrings.github.io/griffe/guide/users/how-to/selectively-inspect/. It will show you how to selectively "inspect" (use dynamic analysis on) some objects. In this case, you'd inspect these dynamically generated fields you write about. Let me know if it helps, or if anything is unclear in the how-to.

Yes I came across that, but if I'm going to write code I'd rather work on a PR for this package. I would expect the dynamic fallback automatically, or at least some configuration option to switch to dynamic instead of static. Am I understanding it right that there is no way to tell griffe_pydantic to use dynamic analysis currently? When is dynamic.py actually used?

I'm also interested in getting more info on this ast.literal_eval crash! Could you open a new issue with the traceback and all? That'd be great 🙂

Sure, I can give a pretty minimal example. Should I open it as a bug report?

@nardi
Copy link
Author

nardi commented Dec 2, 2024

I made a bug report here.

@pawamoy
Copy link
Member

pawamoy commented Dec 2, 2024

Yes I came across that, but if I'm going to write code I'd rather work on a PR for this package. I would expect the dynamic fallback automatically, or at least some configuration option to switch to dynamic instead of static.

I see, thanks. I don't think we will integrate such a feature into Griffe directly. Not in the loader logic, and not as a built-in extension. The reason is, there are infinite ways...

  • in which static analysis can be insufficient
  • to detect this insufficiency
  • to correct it with dynamic analysis

But! I'm open to the idea of providing an official extension (in a separate repository of the mkdocstrings organization) that does the most simple thing: replace statically obtained objects with dynamically obtained ones, with an inclusion list. Basically the last example in the how-to mentioned above. This way users won't have to reimplement it in every project.

Or you could write it once and publish it on PyPI, too, as a third-party extension. I'd be happy to list it in our docs, next to the other third-party extensions 🙂

Am I understanding it right that there is no way to tell griffe_pydantic to use dynamic analysis currently? When is dynamic.py actually used?

The griffe-pydantic extension will follow what the agent above is doing: static or dynamic. For example if we were to run Griffe with the "force inspection" option enabled, the extension would use dynamic analysis too.

@nardi
Copy link
Author

nardi commented Dec 2, 2024

I see, thanks. I don't think we will integrate such a feature into Griffe directly. Not in the loader logic, and not as a built-in extension. The reason is, there are infinite ways...

* in which static analysis can be insufficient

* to detect this insufficiency

* to correct it with dynamic analysis

Ok, that is too bad. This situation seems pretty clear to me (if ast.literal_eval cannot evaluate it, the object has to be imported dynamically) but I can imagine to do this holistically is more difficult.

But! I'm open to the idea of providing an official extension (in a separate repository of the mkdocstrings organization) that does the most simple thing: replace statically obtained objects with dynamically obtained ones, with an inclusion list. Basically the last example in the how-to mentioned above. This way users won't have to reimplement it in every project.

Or you could write it once and publish it on PyPI, too, as a third-party extension. I'd be happy to list it in our docs, next to the other third-party extensions 🙂

Ok, that makes sense. I will have a look :)

The griffe-pydantic extension will follow what the agent above is doing: static or dynamic. For example if we were to run Griffe with the "force inspection" option enabled, the extension would use dynamic analysis too.

Ok, that is interesting. If I run the following code:

dynamic_model = """
import pydantic

desc = "xyz"

class TestModel(pydantic.BaseModel):
    abc: str = pydantic.Field(description=desc)
"""

with griffe.temporary_pypackage("package", modules={"__init__.py": dynamic_model}) as tmp_package:
    package = griffe.load(
        tmp_package.name,
        search_paths=[tmp_package.tmpdir],
        extensions=griffe.load_extensions("griffe_pydantic"),
        force_inspection=True,
    )

    assert package["TestModel"]["abc"].docstring.value == "xyz"

This also doesn't work:

---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
Cell In[3], line 34
     26 with griffe.temporary_pypackage("package", modules={"__init__.py": dynamic_model}) as tmp_package:
     27     package = griffe.load(
     28         tmp_package.name,
     29         search_paths=[tmp_package.tmpdir],
     30         extensions=griffe.load_extensions("griffe_pydantic"),
     31         force_inspection=True,
     32     )
---> 34     assert package["TestModel"]["abc"].docstring.value == "xyz"

File [...]\_griffe\mixins.py:61, in GetMembersMixin.__getitem__(self, key)
     59 parts = _get_parts(key)
     60 if len(parts) == 1:
---> 61     return self.all_members[parts[0]]  # type: ignore[attr-defined]
     62 return self.all_members[parts[0]][parts[1:]]

KeyError: 'abc'

Is that another bug?

@pawamoy
Copy link
Member

pawamoy commented Dec 2, 2024

Hmmm. Rather lack of support for Pydantic v2 🤔?

The BaseModel class moves fields into the model_fields attribute (dictionary). Well, they're probably actually stored somewhere else, in an __pydantic_stuff__ attribute, and model_fields is just the public API.

>>> TestModel.abc
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/media/data/.cache/uv/archive-v0/8uS4XdTqaXfCDG3gRHApm/lib/python3.12/site-packages/pydantic/_internal/_model_construction.py", line 320, in __getattr__
    raise AttributeError(item)
AttributeError: abc

The griffe-pydantic extension should obviously support that but I didn't have a lot of time to improve it, and Pydantic v2 came out in the meantime so it probably doesn't support it very well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants