Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Upgrading to Pydantic v2 and DiffSync v2 #327

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

theandrelima
Copy link

@theandrelima theandrelima commented Jul 4, 2024

Overview

network-importer's dependency on pydantic = "^1.6" limits users who need to use other packages dependent on Pydantic 2.x in the same Python environment.

The original motivation behind this PR was to move network-importer's dependency to Pydantic v2.x. However, network-importer depends on DiffSync, which in turn also depends on Pydantic.

For that, moving to DiffSync v2 is also required for compatibility with Pydantic v2.

Changes Made

  • Upgraded to Pydantic v2.
  • Refactored settings classes to use pydantic-settings.
  • Applied overall code fixes for Pydantic v2 compatibility.
  • Upgraded to DiffSync v2 and made necessary code adjustments for breaking changes.
  • Proposed project version bump to 4.0.0 (subject to NTC's discretion) due to breaking changes.

Testing

  • All tests passed using the invoke pytest command. (see Additional Notes below)
  • Manual testing in CLI mode against a dev instance of our Nautobot environment confirmed the functionality both in check and apply modes, and with/without --update-configs and --limit flags.

Additional Notes

invoke pytest only worked after I added the following line to the end of the Dockerfile: RUN pip install .

I'm not sure why, but lines 14 and 15 of the current Dockerfile were not taking the effect of making network_importer itself available in the python environment of the resulting built image.

I decided to not tackle this at all within this PR because:
1 - It might be some weirdness with my own sandbox environment.
2 - Even if it is not, I believe it is out-of-scope.

@msheiny
Copy link

msheiny commented Dec 24, 2024

Looks like this is failing because of the removed docstrings in BatfishSettings and NetworkSettings classes:

./network_importer/config.py:41 in public class `BatfishSettings`:
        D101: Missing docstring in public class
./network_importer/config.py:51 in public class `NetworkSettings`:
        D101: Missing docstring in public class

@jvanderaa
Copy link
Contributor

I'm onboard with getting this merged in and let's then take a pass at getting the rest updated.

For that update (not this one), @theandrelima would we be good to update Python minimums in your environment? Only because Pandas and Numpy are aggressively moving forward.

@theandrelima
Copy link
Author

theandrelima commented Jan 1, 2025

I'm onboard with getting this merged in and let's then take a pass at getting the rest updated.

For that update (not this one), @theandrelima would we be good to update Python minimums in your environment? Only because Pandas and Numpy are aggressively moving forward.

I was off for a couple of weeks. Back now and happy to see that this might get merged soon, hopefully?

All right! tWill do.
But just to confirm, that is to happen in a separate PR, right?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants