feat: tweaks for gremlin support #60

friendtocephalopods · 2020-09-14T21:34:22Z

Summary of Changes

add optional key parameter to column, table. rename Statistics class to Stat.
add fixtures, streaming, utils to common.

Update mypy to 761 to fix false errors.
Added ignore_missing_imports to ignore marshmallow dependency typing complaints.
Fix flake8 complaints about extra line in init.py files.
Added type ignore for thorny mypy complaint in User.py

As with the rest of the gremlin code I'm upstreaming, this was a team effort. Kudos to @dsimms, @alran, @worldwise001.

CheckList

Make sure you have checked all steps below to ensure a timely review.

PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.
PR includes a summary of changes.

add optional key parameter to column, table. rename Statistics -> Stat. add fixtures, streaming, utils to common. Update mypy to 761 to fix false errors. Fix flake8 complaints about extra line in __init__.py files Signed-off-by: Joshua Hoskins <hoskins@squareup.com>

alran · 2020-09-15T21:28:16Z

amundsen_common/fixtures.py

+
+
+class Fixtures:
+    counter = 1000


It might be useful to add some documentation somewhere on how to use this Fixtures class (a summary of some kind so that people don't have to read all the code to figure it out)

document fixtures

alran · 2020-09-15T21:30:17Z

This looks really good to me @friendtocephalopods! Had one comment about potentially adding more documentation for the casual browser. Also I know that I have tried to upstream that Statistics -> Stat change before. I'd love to see it go through :)

cpu · 2020-09-16T16:13:59Z

This looked good to me as well. Nice work!

One top-level question: why the rename of the Statistics class? It might be nice to call out the rationale for that change since it's less obvious than the other diffs in this branch (imo).

friendtocephalopods · 2020-09-16T16:32:53Z

This looked good to me as well. Nice work!

One top-level question: why the rename of the Statistics class? It might be nice to call out the rationale for that change since it's less obvious than the other diffs in this branch (imo).

Oh, good question! Each Statistics class just contains just one statistic. Additionally, the parameters are all named stat_x and the field is referenced as a list as stats -- so Stat seems a bit more appropriate.

feng-tao

thanks for putting up the pr. haven't time to go through, put some initial comments, let me know what you think.

feng-tao · 2020-09-16T16:45:30Z

amundsen_common/fixtures.py

+from amundsen_common.models.user import User
+
+
+class Fixtures:


Is the fixtures only for testing purpose? if so, we should move it under the tests folder.

Ah, yes, it's for testing! But it's intended to be useful in multiple repos to create test objects easily. I don't think the tests/ folder gets carried over cleanly when importing in other repos.

Maybe amundsen_common/tests/fixtures.py would be preferable?

yeah, amundsen_common/tests/fixtures.py would be better.

amundsen_common/tests/fixtures.py

feng-tao · 2020-09-16T16:49:26Z

amundsen_common/models/table.py

@@ -61,26 +61,27 @@ class Meta:


 @attr.s(auto_attribs=True, kw_only=True)
-class Statistics:
+class Stat:


unfortunately, this will break all the other services (https://github.com/amundsen-io/amundsenmetadatalibrary/search?q=Statistics&unscoped_q=Statistics) . I know this is not perfect. But let's keep it as it is and do it differently (e.g add another class named Stat and mark Statistics deprecated etc)

So the suggestion is to change it all over to a new Stat class here and only remove Statistics after references are gone in the other repos? Sure, that works!

new class, not rename

feng-tao · 2020-09-16T16:49:46Z

amundsen_common/models/table.py

    stat_type: str
    stat_val: Optional[str] = None
    start_epoch: Optional[int] = None
    end_epoch: Optional[int] = None


-class StatisticsSchema(AttrsSchema):
+class StatSchema(AttrsSchema):


same for the rename

feng-tao · 2020-09-16T16:49:59Z

amundsen_common/models/table.py

@@ -151,6 +153,7 @@ class Table:
    cluster: str
    schema: str
    name: str
+    key: Optional[str] = None


could you share more what the key is for?

Sure! This is the table_uri. We store it on the object in the graph for indexing anyhow, so this saves us from having to recompute it everywhere each time we fetch it.

If we fully roll this out and make it non-optional, it also lets us not recompute the key downstream in the frontend as well: https://github.com/amundsen-io/amundsenfrontendlibrary/blob/ee0474fdcce6c8c1b01c9032b5cd2750b1d0d92c/amundsen_application/api/utils/metadata_utils.py#L110

feng-tao · 2020-09-16T16:51:15Z

amundsen_common/utils/streams.py

+    return 1
+
+
+class PeekingIterator(Iterator[V]):


so I don't have much context for all the code. If it is for gremlin lib, could we move it to https://github.com/amundsen-io/amundsengremlin ? The common lib is only used to store common model for all the repos. Let me know what you thinks.

Yeah...I was wondering myself if this is useful in anyone else's databuilder. We use this in the gremlin code, but also for some of our databuilder jobs to chunk the results (e.g. groups of 1,000 or 10,000. Unlimited size batches can be problematic in some circumstances). Do y'all ever use airflow to explicitly set batch sizes?

We define the batch / transaction size through config during neo4j ingestion (https://github.com/amundsen-io/amundsendatabuilder/blob/master/databuilder/publisher/neo4j_csv_publisher.py#L95). Otherwise the neo4j will hang without batch size setting. So given we haven't had a clear use case, I would suggest moving the code to gremlin repo? we could always use gremlin as an extra library deps in databuilder if we plan to enhance and build neptune publisher there. cc @AndrewCiambrone

Sure, sounds good to me! Thanks for the colour, Tao!

streams to gremlin repo

Remove streams Move fixtures to amundsen_common/tests Stat as additional class, deprecate Statistics Signed-off-by: Joshua Hoskins <hoskins@squareup.com>

feng-tao

lgtm mostly, a few more nits, let me know what you think

feng-tao · 2020-09-17T20:53:02Z

setup.cfg

@@ -35,6 +35,7 @@ check_untyped_defs = true
 disallow_any_generics = true
 disallow_incomplete_defs = true
 disallow_untyped_defs = true
+ignore_missing_imports = true


could you share more on why we set this?

Of course! When I run mypy on the entire repo, I get it complaining at me about three libraries missing stubs.

e.g.

setup.py:4: error: No library stub file for module 'setuptools' amundsen_common/models/user.py:7: error: Cannot find implementation or library stub for module named 'marshmallow' amundsen_common/models/user.py:8: error: Cannot find implementation or library stub for module named 'marshmallow_annotations.ext.attrs'

I can set it specifically for those libraries in the cfg though, which is a less stringent option.

feng-tao · 2020-09-17T20:53:30Z

amundsen_common/utils/utils.py

+# Copyright Contributors to the Amundsen project.
+# SPDX-License-Identifier: Apache-2.0
+
+from typing import Optional, TypeVar


could we move the utils folder to gremlin as well ?

Sure!

move the utils folder to gremlin

feng-tao · 2020-09-17T20:53:35Z

amundsen_common/utils/__init__.py

@@ -0,0 +1,2 @@
+# Copyright Contributors to the Amundsen project.
+# SPDX-License-Identifier: Apache-2.0


feng-tao · 2020-09-17T20:54:18Z

amundsen_common/models/table.py

@@ -151,6 +153,7 @@ class Table:
    cluster: str
    schema: str
    name: str
+    key: Optional[str] = None


remove utils, make mypy ignore missing imports module-specific Signed-off-by: Joshua Hoskins <hoskins@squareup.com>

feng-tao · 2020-09-17T21:17:01Z

thanks

alran · 2020-09-17T21:23:38Z

🙏

friendtocephalopods force-pushed the add-fixtures branch 2 times, most recently from 934e115 to 7cb2175 Compare September 14, 2020 22:00

friendtocephalopods marked this pull request as ready for review September 14, 2020 22:07

friendtocephalopods requested review from allisonsuarez, bolkedebruin, dikshathakur3119, feng-tao, jinhyukchang, mgorsk1, verdan and a team as code owners September 14, 2020 22:07

friendtocephalopods force-pushed the add-fixtures branch from 7cb2175 to 86a0704 Compare September 15, 2020 21:22

friendtocephalopods force-pushed the add-fixtures branch from 86a0704 to 013735c Compare September 15, 2020 21:25

alran reviewed Sep 15, 2020

View reviewed changes

feng-tao reviewed Sep 16, 2020

View reviewed changes

Feedback 1

06f1f73

Remove streams Move fixtures to amundsen_common/tests Stat as additional class, deprecate Statistics Signed-off-by: Joshua Hoskins <hoskins@squareup.com>

friendtocephalopods force-pushed the add-fixtures branch from b7e75ef to 06f1f73 Compare September 16, 2020 18:22

friendtocephalopods requested a review from feng-tao September 16, 2020 20:27

feng-tao reviewed Sep 17, 2020

View reviewed changes

feedback 2

3f1557a

remove utils, make mypy ignore missing imports module-specific Signed-off-by: Joshua Hoskins <hoskins@squareup.com>

friendtocephalopods force-pushed the add-fixtures branch from a318f64 to 3f1557a Compare September 17, 2020 21:11

friendtocephalopods requested a review from feng-tao September 17, 2020 21:13

feng-tao approved these changes Sep 17, 2020

View reviewed changes

feng-tao merged commit 1a2733b into amundsen-io:master Sep 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: tweaks for gremlin support #60

feat: tweaks for gremlin support #60

friendtocephalopods commented Sep 14, 2020 •

edited

Loading

alran Sep 15, 2020

friendtocephalopods Sep 16, 2020 •

edited

Loading

alran commented Sep 15, 2020

cpu commented Sep 16, 2020

friendtocephalopods commented Sep 16, 2020

feng-tao left a comment

feng-tao Sep 16, 2020

friendtocephalopods Sep 16, 2020

feng-tao Sep 16, 2020

friendtocephalopods Sep 16, 2020 •

edited

Loading

feng-tao Sep 16, 2020

friendtocephalopods Sep 16, 2020 •

edited

Loading

feng-tao Sep 16, 2020

feng-tao Sep 16, 2020

friendtocephalopods Sep 16, 2020

feng-tao Sep 17, 2020

feng-tao Sep 16, 2020

friendtocephalopods Sep 16, 2020 •

edited

Loading

feng-tao Sep 16, 2020

friendtocephalopods Sep 16, 2020 •

edited

Loading

feng-tao left a comment

feng-tao Sep 17, 2020

friendtocephalopods Sep 17, 2020

feng-tao Sep 17, 2020

friendtocephalopods Sep 17, 2020 •

edited by feng-tao

Loading

feng-tao Sep 17, 2020

feng-tao Sep 17, 2020

feng-tao commented Sep 17, 2020

alran commented Sep 17, 2020

		@@ -0,0 +1,2 @@
		# Copyright Contributors to the Amundsen project.
		# SPDX-License-Identifier: Apache-2.0

feat: tweaks for gremlin support #60

feat: tweaks for gremlin support #60

Conversation

friendtocephalopods commented Sep 14, 2020 • edited Loading

Summary of Changes

CheckList

Choose a reason for hiding this comment

friendtocephalopods Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

alran commented Sep 15, 2020

cpu commented Sep 16, 2020

friendtocephalopods commented Sep 16, 2020

feng-tao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

friendtocephalopods Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

friendtocephalopods Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

friendtocephalopods Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

friendtocephalopods Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

feng-tao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

friendtocephalopods Sep 17, 2020 • edited by feng-tao Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feng-tao commented Sep 17, 2020

alran commented Sep 17, 2020

friendtocephalopods commented Sep 14, 2020 •

edited

Loading

friendtocephalopods Sep 16, 2020 •

edited

Loading

friendtocephalopods Sep 16, 2020 •

edited

Loading

friendtocephalopods Sep 16, 2020 •

edited

Loading

friendtocephalopods Sep 16, 2020 •

edited

Loading

friendtocephalopods Sep 16, 2020 •

edited

Loading

friendtocephalopods Sep 17, 2020 •

edited by feng-tao

Loading