dan-fritchman / Hdl21

Hardware Description Library
BSD 3-Clause "New" or "Revised" License
69 stars 16 forks source link

FIX: Pydantic v2 patch upgrade #180

Closed daquintero closed 1 year ago

daquintero commented 1 year ago

Hi Dan,

Hope you are well!

hdl21 is conflicting with some other packages that have updated to pydantic v2. I've tried to do the proper migration, only to realise it is quite involved and will take me far longer than I can.

However, instead, what I have done in this PR is to do the patch migration where everything stays compatible to the original version using the pydantic.v1 submodule they provide. This makes the migration much easier and can be progressively implemented if required.

This is still a draft because when I ran the pytest I got this:

>       return vlsir.Prefixed(string_value=str(pref.number), prefix=prefix)
E       ValueError: Protocol message Prefixed has no "string_value" field.

hdl21\proto\exporting.py:434: ValueError
======================================================================================================= short test summary info =======================================================================================================
FAILED examples/test_examples.py::test_ro - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED examples/test_examples.py::test_rdac - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED examples/test_examples.py::test_diff_ota - ValueError: Protocol message ParamValue has no "int64_value" field.
FAILED examples/test_examples.py::test_idac - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED hdl21/pdk/sample_pdk/test_sample_pdk.py::test_netlist - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED hdl21/sim/tests/test_sim.py::test_sim2 - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED hdl21/sim/tests/test_sim.py::test_simattrs - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED hdl21/sim/tests/test_sim.py::test_sim_decorator - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED hdl21/sim/tests/test_sim.py::test_proto1 - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED hdl21/sim/tests/test_sim.py::test_delay1 - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED hdl21/tests/test_builtin_generators.py::test_mos_generator - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED hdl21/tests/test_exports.py::test_prim_proto1 - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED hdl21/tests/test_exports.py::test_ideal_primitives - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED hdl21/tests/test_exports.py::test_spice_netlister - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED hdl21/tests/test_exports.py::test_roundtrip_external_module - ValueError: Protocol message ParamValue has no "int64_value" field.
FAILED pdks/Gf180/gf180_hdl21/test_netlists.py::test_xtor_netlists - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED pdks/Gf180/gf180_hdl21/test_netlists.py::test_2_term_res_netlists - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED pdks/Gf180/gf180_hdl21/test_netlists.py::test_3_term_res_netlists - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED pdks/Gf180/gf180_hdl21/test_netlists.py::test_diode_netlists - ValueError: Protocol message Prefixed has no "string_value" field.
FAILED pdks/Gf180/gf180_hdl21/test_netlists.py::test_3T_bjt_netlists - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED pdks/Gf180/gf180_hdl21/test_netlists.py::test_4T_bjt_netlists - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED pdks/Gf180/gf180_hdl21/test_netlists.py::test_cap_netlists - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED pdks/Gf180/gf180_hdl21/test_pdk.py::test_netlist - ValueError: Protocol message Prefixed has no "string_value" field.
FAILED pdks/Sky130/sky130_hdl21/test_netlists.py::test_xtor_netlists - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED pdks/Sky130/sky130_hdl21/test_netlists.py::test_2_term_res_netlists - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED pdks/Sky130/sky130_hdl21/test_netlists.py::test_3_term_res_netlists - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED pdks/Sky130/sky130_hdl21/test_netlists.py::test_diode_netlists - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED pdks/Sky130/sky130_hdl21/test_netlists.py::test_pnp_netlists - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED pdks/Sky130/sky130_hdl21/test_netlists.py::test_npn_netlists - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED pdks/Sky130/sky130_hdl21/test_netlists.py::test_mim_cap_netlists - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED pdks/Sky130/sky130_hdl21/test_netlists.py::test_var_cap_netlists - ValueError: Protocol message Prefixed has no "int64_value" field.
FAILED pdks/Sky130/sky130_hdl21/test_pdk.py::test_netlist - ValueError: Protocol message Prefixed has no "string_value" field.
======================================================================================== 32 failed, 180 passed, 4 skipped, 5 xfailed in 3.69s ==================================

However, as far as I can tell, these are all issues in vlsirtools when tested from hdl21. The latest github versions are straightup incompatible, so this is when using vlsirtools==4.0.0 release.

What do you think? I'm going to try to keep debugging to get the tests to pas. They're all related to int64_value fields so maybe that makes some sense to you. It'd be nice to have a patch release of this in pypi when we get tests to pass so that hdl21 can be used in a compatible environment with the other pydantic v2 tools.

Re https://github.com/dan-fritchman/Hdl21/issues/157 and maybe https://github.com/dan-fritchman/Hdl21/issues/174

daquintero commented 1 year ago

I've realised this has to do with the latest protobuffer changes, so I don't think it necessarily has to do with the pydantic upgrade.

dario@DESKTOP-1T2BQKV MINGW64 ~/Documents/phd/software/Hdl21 (pydantic_v2_patch_upgrade)
$ python -m pytest
ImportError while loading conftest 'C:\Users\dario\Documents\phd\software\Hdl21\conftest.py'.
conftest.py:8: in <module>
    from vlsirtools.pytest import (                                          
..\vlsir\vlsirtools\vlsirtools\__init__.py:8: in <module>                    
    from . import spice                                                      
..\vlsir\vlsirtools\vlsirtools\spice\__init__.py:2: in <module>              
    from . import spectre                                                    
..\vlsir\vlsirtools\vlsirtools\spice\spectre.py:15: in <module>              
    from ..netlist.spectre import SpectreNetlister                           
..\vlsir\vlsirtools\vlsirtools\netlist\__init__.py:16: in <module>           
    from .spectre import SpectreNetlister                                    
..\vlsir\vlsirtools\vlsirtools\netlist\spectre.py:14: in <module>            
    from .spectre_spice_shared import SpectreSpiceShared                     
..\vlsir\vlsirtools\vlsirtools\netlist\spectre_spice_shared.py:2: in <module>
    from .base import Netlister
..\vlsir\vlsirtools\vlsirtools\netlist\base.py:13: in <module>
    from .. import primitives
..\vlsir\vlsirtools\vlsirtools\primitives.py:255: in <module>
    value=ParamValue(int64_value=0),
dan-fritchman commented 1 year ago

That's right, there was a recent VLSIR schema change, particularly to a few field-names that clashed with other applications. The head-of-GitHub VLSIR should work since #172.

Re: the pydantic v2 - if it works, sure. I am fairly wary since:

Might I ask, what is/ are the library(s) already using Pydantic v2 that are clashing?

daquintero commented 1 year ago

Ahh thanks for the merge of the fix! I am trying it now.

In here they describe that this notation should mean pydantic v1 is available within pydantic v2, but I know it introduces breaking changes with the v1 original dependency, as this version is v1 inside v2. I'd be happy to help port the project you care about too if it's public! Unsure how to test best those subtle changes!

Unfortunately it is the latest versions >7.3 of gdsfactory and the collection of gplugins which I use quite often with hdl21 so it affects a fair bit when using both tools! Would it work to have a 4.0.1alpha version for the future pydantic upgrade so that when the time comes this helps port to it and is just a pre-release but does not replace 4.0.0? I think that might work but sorry to ask putting a bit more work for you. Would be most appreciated!

dan-fritchman commented 1 year ago

OK thanks.
And sadly I don't expect the peer project is gonna sign up for your contributions. Much less to a particular schedule.

I can imagine Hdl21 (or any similar library) detecting which version it has and reacting with something like:

# In, like, hdl21/our_pydantic_stuff.py
import pydantic 

# Sort out which namespace to use
if figure_out_version_2_or_greater():
  our_pydantic_stuff = pydantic.v1
else:
  our_pydantic_stuff = pydantic

# "Export" the stuff we want to expose directly in this module namespace
dataclass = our_pydantic_stuff.dataclass 
BaseModel = our_pydantic_stuff.BaseModel
# ... all other such things we use 
# Everywhere else in Hdl21

## from pydantic.dataclasses import dataclass ## <= No! Bad! 
from .our_pydantic_stuff import dataclass ## <= that's the idea

Does, like, their migration tool do something like this already?

I also don't know whether those "v1" types actually work when bundled in with v2. But you could try.

daquintero commented 1 year ago

Ahh fair enough!

I think your idea is good in the way of supporting multiple versions. Their bump version is like a lexical changer of code, but does not quite evaluate the usage of the classes - I tried on this already and it didn't edit anything.

I'll come up with a clever way of doing what you're suggesting and making it work and make a new PR! Thanks for the feedback