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

DoltTable Plugin #461

Merged
merged 35 commits into from
May 3, 2021
Merged

DoltTable Plugin #461

merged 35 commits into from
May 3, 2021

Conversation

max-hoffman
Copy link
Contributor

@max-hoffman max-hoffman commented Apr 29, 2021

Signed-off-by: Max Hoffman [email protected]

TL;DR

Adds a DoltTable -- wrapper around pandas.DataFrame that commits changes to versioned database.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

hanging TODO's:

  • code coverage
  • metadata management?
  • flytesnacks (separate PR)

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

https://github.com/lyft/flyte/issues/

Follow-up issue

NA
OR
https://github.com/lyft/flyte/issues/

@max-hoffman max-hoffman force-pushed the dolt/table branch 2 times, most recently from 0a3dcae to e38d3fe Compare April 29, 2021 05:05
@max-hoffman max-hoffman changed the title Initial DoltTable implementation DoltTable Plugin Apr 29, 2021
Comment on lines 10 to 12
apt-get update -y \
&& apt-get install curl \
&& sudo bash -c 'curl -L https://github.com/dolthub/dolt/releases/latest/download/install.sh | sudo bash'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wild-endeavor Something i was confused about before -- will CI run this script to install Dolt?

Copy link
Contributor

Choose a reason for hiding this comment

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

CI will run this https://github.com/flyteorg/flytekit/blob/master/.github/workflows/pythonbuild.yml#L37 so as long as it gets picked up from that we should be good.

I'm also thinking we'll need to move plugins into a separate repo sooner rather than later.

Copy link
Contributor

@wild-endeavor wild-endeavor left a comment

Choose a reason for hiding this comment

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

Can we rename the plugin from dolt to flytedolt or something? I need to rename sqlalchemy actually to "flytesqlalchemy". Under certain installations of pip dependencies, it thinks that the plugin is the real sqlalchemy instead, so it didn't pick up the real library.

@max-hoffman
Copy link
Contributor Author

@wild-endeavor would something like this solve those problems?
image
I'm surprised the other plugins like papermill and pandera haven't had issues

@wild-endeavor
Copy link
Contributor

is that common practice? using . in the name of the plugin?

@max-hoffman
Copy link
Contributor Author

I think this would work too:

plugins/
    flytekitplugins.dolt/
        setup.py
        flytekitplugins/
            dolt/
                __init__.py

I'm not confident about how different namespacing formats impact pip clashes.

jeevb and others added 18 commits April 29, 2021 13:22
* Move configuraiton of a few tools to pyproject.toml

Signed-off-by: Hongxin Liang <[email protected]>

* Stop pinning black

Formatted by later version of black

Signed-off-by: Hongxin Liang <[email protected]>
Signed-off-by: Max Hoffman <[email protected]>
* CI for spark2

Signed-off-by: Hongxin Liang <[email protected]>

* Simplify Makefile

Signed-off-by: Hongxin Liang <[email protected]>
Signed-off-by: Max Hoffman <[email protected]>
* add requests and limits parameter

Signed-off-by: Miguel Toledo <[email protected]>

* fix typo

Signed-off-by: Miguel Toledo <[email protected]>

* signoff

Signed-off-by: Miguel Toledo <[email protected]>
Signed-off-by: Max Hoffman <[email protected]>
* update flytekit docs theme, fix index links

Signed-off-by: cosmicBboy <[email protected]>

* add readthedocs sphinx search

Signed-off-by: cosmicBboy <[email protected]>

* update flyteidl link

Signed-off-by: cosmicBboy <[email protected]>

* update overview and homepage

Signed-off-by: cosmicBboy <[email protected]>

* address comments @wild-endeavor

Signed-off-by: cosmicBboy <[email protected]>

* update ipykernel

Signed-off-by: cosmicBboy <[email protected]>

* add community link

Signed-off-by: cosmicBboy <[email protected]>
Signed-off-by: Max Hoffman <[email protected]>
Signed-off-by: cosmicBboy <[email protected]>
Signed-off-by: Max Hoffman <[email protected]>
* Added a missing test for != `ne` condition

Signed-off-by: Ketan Umare <[email protected]>

* improved the test to use previous task output

Signed-off-by: Ketan Umare <[email protected]>
Signed-off-by: Max Hoffman <[email protected]>
Signed-off-by: wild-endeavor <[email protected]>
Signed-off-by: Max Hoffman <[email protected]>
* implement new control plane classes

Signed-off-by: cosmicBboy <[email protected]>

* revert dep changes

Signed-off-by: cosmicBboy <[email protected]>

* remove unneeded mock integration test files

Signed-off-by: cosmicBboy <[email protected]>

* remove pytest.ini

Signed-off-by: cosmicBboy <[email protected]>

* add integration tests to ci, update reqs

Signed-off-by: cosmicBboy <[email protected]>

* add unit tests

Signed-off-by: cosmicBboy <[email protected]>

* lint

Signed-off-by: cosmicBboy <[email protected]>

* address comments @wild-endeavor

Signed-off-by: cosmicBboy <[email protected]>
Signed-off-by: Max Hoffman <[email protected]>
cosmicBboy and others added 5 commits April 29, 2021 13:22
* fix imports

Signed-off-by: cosmicBboy <[email protected]>

* update

Signed-off-by: cosmicBboy <[email protected]>
Signed-off-by: Max Hoffman <[email protected]>
Signed-off-by: Max Hoffman <[email protected]>
Signed-off-by: Max Hoffman <[email protected]>
Signed-off-by: Max Hoffman <[email protected]>
Signed-off-by: Max Hoffman <[email protected]>
Signed-off-by: Max Hoffman <[email protected]>
Signed-off-by: Max Hoffman <[email protected]>
Signed-off-by: Max Hoffman <[email protected]>
Signed-off-by: Max Hoffman <[email protected]>
Signed-off-by: Max Hoffman <[email protected]>
Signed-off-by: Max Hoffman <[email protected]>
@wild-endeavor
Copy link
Contributor

yeah i like the second approach. i think the top level one is the one messing things up.

Signed-off-by: Max Hoffman <[email protected]>
@codecov
Copy link

codecov bot commented Apr 30, 2021

Codecov Report

Merging #461 (fa298da) into master (8492223) will increase coverage by 0.07%.
The diff coverage is 97.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #461      +/-   ##
==========================================
+ Coverage   84.36%   84.43%   +0.07%     
==========================================
  Files         357      360       +3     
  Lines       26630    26780     +150     
  Branches     2206     2211       +5     
==========================================
+ Hits        22467    22613     +146     
- Misses       3564     3566       +2     
- Partials      599      601       +2     
Impacted Files Coverage Δ
...lytekitplugins.dolt/flytekitplugins/dolt/schema.py 92.98% <92.98%> (ø)
...tekitplugins.dolt/flytekitplugins/dolt/__init__.py 100.00% <100.00%> (ø)
plugins/tests/dolt/test_wf.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 8492223...fa298da. Read the comment docs.

wild-endeavor
wild-endeavor previously approved these changes Apr 30, 2021
Copy link
Contributor

@wild-endeavor wild-endeavor left a comment

Choose a reason for hiding this comment

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

+1 but unf you have to fix some conflicts. I'll approve again after. just ping me.

Signed-off-by: Max Hoffman <[email protected]>
Signed-off-by: Max Hoffman <[email protected]>
@wild-endeavor wild-endeavor merged commit 2340bfa into flyteorg:master May 3, 2021
@max-hoffman max-hoffman deleted the dolt/table branch May 3, 2021 16:36
max-hoffman added a commit to dolthub/flytekit that referenced this pull request May 11, 2021
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.

9 participants