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

Import modules/objects directly from v1 namespace #9162

Open
wants to merge 9 commits into
base: 1.10.X-fixes
Choose a base branch
from

Conversation

exs-dwoodward
Copy link

@exs-dwoodward exs-dwoodward commented Apr 4, 2024

Change Summary

Following on from this PR about adding a v1.py namespace to pydantic to better follow the v2 syntax of importing v1 objects, imports such as:

from pydantic.fields import ModelField
from pydantic.generics import GenericModel

or:

from pydantic import fields
from pydantic import generics

cannot be directly swapped for:

from pydantic.v1.fields import ModelField
from pydantic.v1.generics import GenericModel

or:

from pydantic.v1 import fields
from pydantic.v1 import generics

imports. If this was the case, this would make unpinning from v1 --> v2 much simpler as a simple find and replace would be all that is needed to convert from pydantic --> from pydantic.v1 instead (ofc still using v1 types and models).

One caveat with this approach is if this happens:

from pydantic.fields import ModelField
from pydantic.v1.fields import ModelField as ModelFieldFromV1

ModelField is ModelFieldFromV1 # False

but ideally the import syntax should be changed across a codebase rather than only in some places. Note that anything in the __init__.py pydantic namespace is still imported wholesale:

from pydantic import dataclasses
from pydantic.v1 import v1_dataclasses

dataclasses is v1_dataclasses # True

since these modules are imported directly from the underlying submodules. This also means that for the former import block, the ModelField in this case is imported twice.

Additionally, things like Pylance can't directly resolve the import path for pydantic.v1.fields imports using this syntax, but ideally that will be very transient since as soon as the find and replace is executed, any pydantic<2 pins should be able to be removed wholesale and the import will be resolved in pydantic v2.

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected reviewer: @sydney-runkle

skip change file check

Fix #9357

Selected Reviewer: @PrettyWood

@exs-dwoodward
Copy link
Author

This PR is ready for reviewing, please let me know if there are any other steps I can help out with here! Please review :)

@exs-dwoodward
Copy link
Author

@PrettyWood is there anything else I need to do on my end on this PR? This is my first PR into pydantic so apologies if I've missed an automation somewhere!

@sydney-runkle
Copy link
Member

@exs-dwoodward,

Great, thanks so much for your work! Apologies for the review delay - was focusing on FastUI for a bit. You've done everything perfectly here!

@sydney-runkle sydney-runkle mentioned this pull request Apr 30, 2024
15 tasks
Copy link
Member

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

Great, thanks! We can release this with 1.10.16!

tests/test_v1.py Outdated Show resolved Hide resolved
pydantic/v1.py Outdated Show resolved Hide resolved
@sydney-runkle
Copy link
Member

Working on getting the pipeline all green, will merge when that's done!

tests/test_v1.py Outdated Show resolved Hide resolved
@sydney-runkle
Copy link
Member

@exs-dwoodward,

Hmph, looks like tests are failing in some cases:

_______________________ test_can_import_modules_from_v1 ________________________
  
      def test_can_import_modules_from_v1() -> None:
          """That imports from any module in pydantic can be imported through
          ``pydantic.v1.<module>``"""
          for module_fname in os.listdir(pydantic.__path__[0]):
              if module_fname.startswith('_') or not module_fname.endswith('.py') or module_fname == 'v1.py':
                  continue
              module_name = module_fname[:-3]
      
  >           _ = importlib.import_module(f'pydantic.v1.{module_name}')
  
  /Users/runner/work/pydantic/pydantic/tests/test_v1.py:27: 
  _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
  /Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/importlib/__init__.py:127: in import_module
      return _bootstrap._gcd_import(name[level:], package, level)
  <frozen importlib._bootstrap>:1006: in _gcd_import
      ???
  <frozen importlib._bootstrap>:983: in _find_and_load
      ???
  <frozen importlib._bootstrap>:967: in _find_and_load_unlocked
      ???
  <frozen importlib._bootstrap>:677: in _load_unlocked
      ???
  <frozen importlib._bootstrap_external>:728: in exec_module
      ???
  <frozen importlib._bootstrap>:219: in _call_with_frames_removed
      ???
  _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
  
      import sys
      from configparser import ConfigParser
      from typing import Any, Callable, Dict, List, Optional, Set, Tuple, Type as TypingType, Union
      
  >   from mypy.errorcodes import ErrorCode
  E   ModuleNotFoundError: No module named 'mypy'
  
  /Users/runner/work/pydantic/pydantic/pydantic/mypy.py:5: ModuleNotFoundError
  =========================== short test summary info ============================
  FAILED ../../../../../../../../../Users/runner/work/pydantic/pydantic/tests/test_v1.py::test_can_import_modules_from_v1

@sydney-runkle
Copy link
Member

@exs-dwoodward
Copy link
Author

Thanks @sydney-runkle for looking through it!

hmm weird, running locally all my tests pass, though perhaps I should exclude the mypy module from being imported directly in the import test, given that mypy is an optional install.

I get the feeling that the ImportWarning's on the compiled linux tests seem to be due to the package spec not matching. I wonder if this is due to the relative imports used in pydantic (though I wouldn't want to make a massive change here). An alternative could be to employ a module level __getattr__ that imports the module asked for, but this feels quite heavy handed - wdyt?

@exs-dwoodward
Copy link
Author

oh actually nevermind the __getattr__ can be pretty simple:

import pydantic

def __getattr__(name: str) -> Any:
     return getattr(pydantic, name)

seems to do the trick.

@sydney-runkle
Copy link
Member

@exs-dwoodward,

Hmph, looks like we're still getting some test failures. I'd be ok with a more minimal approach here - not testing mypy or dotenv. Also, could you please fix the linting issue?

Thanks!

@exs-dwoodward
Copy link
Author

exs-dwoodward commented May 1, 2024

I'm not entirely sure there is a nice fix for the linux compiled tests to be honest - not one that I can think of at least :( (that doesn't involve converting all the relative imports in pydantic to absolute) - given it only raises an ImportWarning - and that this 1.10.16 release would only be a temporary bound so that people can easily swap to pydantic 2 type imports, how worried are we about these warning failures? They only seem to crop up after the v1 tests are being run, so I'm wondering if swapping the test order would just magically make them disappear anyway.

@exs-dwoodward exs-dwoodward force-pushed the v1-imports branch 2 times, most recently from 3bc7aa0 to 7a43881 Compare May 7, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants