Skip to content

Comments

Add pydantic and initial sgqlc typesystems#75

Merged
JacobHayes merged 2 commits intogoldenfrom
pydantic-sgqlc-typesystems
Sep 2, 2021
Merged

Add pydantic and initial sgqlc typesystems#75
JacobHayes merged 2 commits intogoldenfrom
pydantic-sgqlc-typesystems

Conversation

@joycex99
Copy link
Contributor

Adds TypeSystems for pydantic and sgqlc. A few known issues/TODOs:

  • the issubclass check in pydantic <-> arti type doesn't always work, e.g. Timestamp.precision is not a class
  • the sgqlc typesystem currently only works with fields that are python classes
  • may want to change the design of including interfaces on Struct in order to capture arbitrary interface(s) on sgqlc Types
  • various mypy tests, as usual.

I'm OOO until Aug 2 or 3, so wanted to put this up before I go.

@joycex99 joycex99 requested review from JacobHayes and mikss July 28, 2021 21:15
Copy link
Member

@JacobHayes JacobHayes left a comment

Choose a reason for hiding this comment

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

Still working through, but I am a bit hesitant about Struct.abstract and particularly Struct.interfaces (but makes sense why you added). Maybe instead of storing that metadata on the Struct, we add extra arguments to the sgqlc TypeSystem/TypeAdapter to_system methods (defaulting to "type" rather than "interface")? That way, users could still convert pydantic -> arti, but then on conversion to sgqlc, choose to make it an Interface or Type (that should be good enough for our backend needs, where we can know when/what we want)?

That would mean roundtripping Interfaces wouldn't work by default, but I think that's probably ok (and likely unavoidable in some cases... unless we add some extra "metadata" field to each Type. I think parquet allows this, which pandas uses to track things like "Categorical", etc)

@joycex99 joycex99 force-pushed the pydantic-sgqlc-typesystems branch from fcd21a8 to 0ec4a12 Compare August 6, 2021 16:59
@codecov
Copy link

codecov bot commented Aug 6, 2021

Codecov Report

Merging #75 (c7caf9b) into golden (265e0bd) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            golden       #75    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           21        23     +2     
  Lines          927      1088   +161     
  Branches       117       136    +19     
==========================================
+ Hits           927      1088   +161     
Impacted Files Coverage Δ
src/arti/internal/models.py 100.00% <100.00%> (ø)
src/arti/types/__init__.py 100.00% <100.00%> (ø)
src/arti/types/pydantic.py 100.00% <100.00%> (ø)
src/arti/types/python.py 100.00% <100.00%> (ø)
src/arti/types/sgqlc.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 265e0bd...c7caf9b. Read the comment docs.

@joycex99 joycex99 force-pushed the pydantic-sgqlc-typesystems branch from 0ec4a12 to 92ff710 Compare August 6, 2021 17:15
@joycex99 joycex99 force-pushed the pydantic-sgqlc-typesystems branch from 92ff710 to cb0d2ff Compare August 13, 2021 20:57
@joycex99 joycex99 marked this pull request as ready for review August 13, 2021 23:03
@JacobHayes JacobHayes force-pushed the pydantic-sgqlc-typesystems branch 3 times, most recently from fb24a38 to f3024b6 Compare August 25, 2021 21:34
@JacobHayes JacobHayes force-pushed the pydantic-sgqlc-typesystems branch from f3024b6 to ff101e3 Compare September 2, 2021 02:58
@JacobHayes JacobHayes force-pushed the pydantic-sgqlc-typesystems branch from ff101e3 to c7caf9b Compare September 2, 2021 03:42
@JacobHayes
Copy link
Member

@joycex99 I went a little overboard on the pydantic type converter, but Timestamp (w/ precision/Literal field) can now be converted:
https://github.com/replicahq/artigraph/blob/c7caf9bd1f124231527fd30cac0f0df192de4598/tests/arti/types/test_pydantic_adapters.py#L39-L44

Gonna go ahead and merge, but happy to have eyes on the second commit when/if you get a chance (some of the hook/hint stuff is a bit odd)!

@JacobHayes JacobHayes merged commit 43036fb into golden Sep 2, 2021
@JacobHayes JacobHayes deleted the pydantic-sgqlc-typesystems branch September 2, 2021 03:51
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