Introduce python view, pickle format, local storage by mikss · Pull Request #62 · artigraph/artigraph

mikss · 2021-07-20T17:20:08Z

This PR introduces simple example implementations of View (Python), Storage (LocalFile), and Format (Pickle).

The View object is instantiated with .read and persisted with .write, but these methods in fact call out to multiple dispatch functions read and write, that implement the expected behavior based on the types of the passed input arguments (namely, view, storage, format).

In principle, the read/write will have to do quite a bit of heavy lifting, potentially using type_system attributes for conversion, but in this lucky example of Python/Local/Pickle, none of that is necessary.

codecov · 2021-07-20T17:21:49Z

Codecov Report

Merging #62 (bddd507) into golden (ce84794) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            golden       #62   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           17        21    +4     
  Lines          619       675   +56     
  Branches        74        75    +1     
=========================================
+ Hits           619       675   +56

Impacted Files	Coverage Δ
src/arti/formats/pickle.py	`100.00% <100.00%> (ø)`
src/arti/internal/utils.py	`100.00% <100.00%> (ø)`
src/arti/storage/local.py	`100.00% <100.00%> (ø)`
src/arti/views/core.py	`100.00% <100.00%> (ø)`
src/arti/views/python.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 ce84794...bddd507. Read the comment docs.

JacobHayes

Looking quite good! Main question is about the View mutation vs return - that may become a bit more clear as we build some of the higher up machinery, but I do have a bit of hesitancy around the mutation (despite arguing for it earlier, iirc 😁).

src/arti/views/core.py

src/arti/views/python.py

JacobHayes · 2021-07-21T05:06:23Z

src/arti/views/python.py

+
+
+@write.register
+def _write(view: Python, format: Pickle, storage: LocalFile) -> None:


Planting this seed (probably should wait to experiment), but iirc multimethod supports class hierarchy based dispatch. In other words, if we built a few mixins (or Protocols, granted multimethod support) for format/storage/view behavior (right now, they're all data), then we could add some high-level read/write handlers that delegate to cover a lot of ground, while still being able to write specific or optimized handlers as needed.

Eg: we could:

add a FileLike mixin/Protocol that has an .open method

make LocalFile subclass FileLike and implement .open

make read/write handlers that have (format: Pickle, storage: FileLike, view: Python) and use storage.open instead of open

mix and match with format/view interface mixins for more generalization OR dive into specific classes (like the current PR) for optimizations

JacobHayes · 2021-07-21T05:25:19Z

src/arti/views/core.py

+
+
+@multidispatch
+def read(view: View, format: Format, storage: Storage) -> View:


What do you think about changing this to -> None and just relying on the view.data assignment? That might be a bit more clear that it works via mutation.

Alternatively, should we get rid of the mutation (can't remember where our prior discussion landed) and return the underlying data/-> Any? I think the challenge (is it one?) we realized was that then write needs an additional data: Any argument

I don't think we like this idea:

Alternatively, should we get rid of the mutation (can't remember where our prior discussion landed) and return the underlying data/-> Any? I think the challenge (is it one?) we realized was that then write needs an additional data: Any argument

Specifically because then the issue of "where is my data" becomes muddled. One side effect is, as you point out, write then needs to find the data before writing. I think the way we have it as-is, a View is an actual object with the data, rather than just an instruction, which smooths over these issues.

I'm still torn because that makes View act a bit different than the strictly metadata Format, Storage, etc (which can be reused across calls to build). For example, if we added a PandasDataFrameView(index=["date"]), than in each call to build (eg: for multiple partitions), we would have to recreate that object w/ the same metadata just to wrap different data.

--

Let's leave View.data is for now and we can reconsider (weighing the costs of an extra write argument) later on.

we would have to recreate that object w/ the same metadata just to wrap different data.

🤷 Is an artifact a thing or a collection of things? Is a view a snapshot of a thing or a collection of snapshots of things? I think if Views support slicing (in this case, rows), different instances per partition feels natural.

* Add NamedTemporaryFile alternative that can be re-opened on Windows * Use NamedTemporaryFile rather than mock_open (to fix breakpoint)

JacobHayes · 2021-07-21T15:47:30Z

Rebased to resolve conflicts + added two tidying commits (1, 2)

JacobHayes

Looking great! Punting on View.data mutation until we build some of the higher up machinery.

--

Next up, we probably need to update Producer.build to accept Views rather than Artifacts directly... Thinking about that a bit, if we expand View to have similar matching as the TypeAdapters, then maybe the build signature can be Annotated[pd.DataFrame, MyArtifact] rather than requiring PandasDataFrame[MyArtifact]

More complex cases may still want to use PandasDataFrame directly to pick a specific View or set additional metadata, but the simple cases would stay simple. 🤔

mikss changed the base branch from golden to trivial_artifact July 20, 2021 17:20

mikss changed the title ~~DRAFT: views~~ RAD-2613: introduce python view, pickle format, local storage Jul 20, 2021

mikss marked this pull request as ready for review July 20, 2021 18:47

mikss requested review from JacobHayes and joycex99 July 20, 2021 18:48

JacobHayes force-pushed the io_format_storage_view branch from 0ea31f8 to a2f07c7 Compare July 20, 2021 19:34

JacobHayes mentioned this pull request Jul 20, 2021

Replace mock_open with temporary file #63

Merged

JacobHayes reviewed Jul 21, 2021

View reviewed changes

JacobHayes force-pushed the trivial_artifact branch from 43d6199 to 809878b Compare July 21, 2021 14:55

Base automatically changed from trivial_artifact to golden July 21, 2021 14:58

mikss and others added 9 commits July 21, 2021 11:44

python view initial pass

886c202

tests for views

5a7b835

type fiddling

0d0ed9c

remove validate method

cbdd81a

change multimethod to multidispatch

3fefac3

Replace mock_open with temporary file (#63)

46a2bdb

* Add NamedTemporaryFile alternative that can be re-opened on Windows * Use NamedTemporaryFile rather than mock_open (to fix breakpoint)

re-order format/storage/view, and move tests

edfd2ce

Remove LocalFile.path default

28f7f31

Check before writing empty data

bddd507

JacobHayes force-pushed the io_format_storage_view branch from cb71a5c to bddd507 Compare July 21, 2021 15:46

JacobHayes approved these changes Jul 21, 2021

View reviewed changes

mikss merged commit 85ff212 into golden Jul 21, 2021

mikss deleted the io_format_storage_view branch July 21, 2021 16:49

JacobHayes mentioned this pull request Jul 21, 2021

move read/write outside of View #65

Merged

JacobHayes changed the title ~~RAD-2613: introduce python view, pickle format, local storage~~ Introduce python view, pickle format, local storage Mar 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Introduce python view, pickle format, local storage#62

Introduce python view, pickle format, local storage#62
mikss merged 9 commits intogoldenfrom
io_format_storage_view

mikss commented Jul 20, 2021 •

edited

Loading

Uh oh!

codecov bot commented Jul 20, 2021 •

edited

Loading

Uh oh!

JacobHayes left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JacobHayes Jul 21, 2021

Uh oh!

JacobHayes Jul 21, 2021

Uh oh!

mikss Jul 21, 2021

Uh oh!

JacobHayes Jul 21, 2021 •

edited

Loading

Uh oh!

mikss Jul 21, 2021

Uh oh!

JacobHayes commented Jul 21, 2021

Uh oh!

JacobHayes left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants



		@write.register
		def _write(view: Python, format: Pickle, storage: LocalFile) -> None:



		@multidispatch
		def read(view: View, format: Format, storage: Storage) -> View:

Comments

Conversation

mikss commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JacobHayes left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JacobHayes Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

JacobHayes Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

mikss Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

JacobHayes Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikss Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

JacobHayes commented Jul 21, 2021

Uh oh!

JacobHayes left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mikss commented Jul 20, 2021 •

edited

Loading

codecov bot commented Jul 20, 2021 •

edited

Loading

JacobHayes left a comment •

edited

Loading

JacobHayes Jul 21, 2021 •

edited

Loading