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 plugin to reduce client imports #287

Merged
merged 11 commits into from
Apr 3, 2024

Conversation

bombsimon
Copy link
Contributor

@bombsimon bombsimon commented Mar 13, 2024

  • Convert input types and return types to ast.Constant
  • Import TYPE_CHECKING and import types if set
  • Import required types inside each method
  • Add tests
  • Update CHANGELOG
  • Update README

There are two generated clients to test this with as many permutations as possible both with and without ShorterResultsPlugin because it had a major effect on the result (and I had some bugs along the way). However the actual code is contained solely in ariadne_codegen/contrib.

This addresses one of the biggest issues from #233. Previously it was very slow to load the clients.py because all types used as input and return values were imported. This adds a plugin that converts them to ast.Constant and only imports the types if we're in a TYPE_CHECKING context.

Example from a project with a couple of hundred methods:

Before

>>> import time
>>> def load():
...     now = time.time()
...     import graphql_client.client
...     print(f"took {time.time() - now}")
...
>>> load()
took 5.043132305145264

After

>>> import time
>>> def load():
...     now = time.time()
...     import graphql_client.client
...     print(f"took {time.time() - now}")
...
>>> load()
took 0.14772796630859375

I wouldn't say it solves the issue because even if this improves imports of the client Pydantic v2 is still very slow. Loading some files with many classes can still take several seconds. I don't think we can solve this without rolling back to Pydantic v1 or wait for an upstream fix though.

* Convert input types and return types to `ast.Constant`
* Import `TYPE_CHECKING` and import types if set
* Import required types inside each method
* Add tests
* Update CHANGELOG
* Update README
@bombsimon bombsimon force-pushed the feat/remove-global-imports-plugin branch from e173d8a to 794743b Compare March 13, 2024 18:22
Comment on lines 169 to 172
ast.ImportFrom(
module=self.imported_classes[import_class_id],
names=[import_class],
),
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 developed this plugin outside of ariadne-codegen so I didn't use any helpers to generate these. Tbh I don't think it helps, it just makes it tricker to read.

Let me know if you'd rather me to import and use all the generators even for plugins or if I can keep this as is.

Copy link
Contributor

@rafalp rafalp left a comment

Choose a reason for hiding this comment

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

Just few nits for now.

README.md Outdated Show resolved Hide resolved
ariadne_codegen/contrib/no_global_imports.py Outdated Show resolved Hide resolved
bombsimon and others added 2 commits March 25, 2024 10:00
Co-authored-by: Rafał Pitoń <rafio.xudb@gmail.com>
README.md Outdated Show resolved Hide resolved
ariadne_codegen/contrib/__init__.py Show resolved Hide resolved
ariadne_codegen/contrib/client_forward_refs.py Outdated Show resolved Hide resolved
@rafalp
Copy link
Contributor

rafalp commented Mar 25, 2024

More nits but I think this will be good to merge after those.

bombsimon and others added 2 commits March 25, 2024 21:57
Co-authored-by: Rafał Pitoń <rafio.xudb@gmail.com>
@rafalp rafalp requested a review from DamianCzajkowski March 28, 2024 15:59
bombsimon and others added 3 commits April 2, 2024 13:14
Co-authored-by: DamianCzajkowski <43958031+DamianCzajkowski@users.noreply.github.com>
@bombsimon bombsimon force-pushed the feat/remove-global-imports-plugin branch from 0454cce to 4e06845 Compare April 2, 2024 15:23
Copy link
Contributor

@DamianCzajkowski DamianCzajkowski left a comment

Choose a reason for hiding this comment

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

LGTM!

@rafalp rafalp merged commit 45b4a20 into mirumee:main Apr 3, 2024
4 checks passed
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.

3 participants