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

Add athena plugin #504

Merged
merged 11 commits into from
Jun 7, 2021
Merged

Add athena plugin #504

merged 11 commits into from
Jun 7, 2021

Conversation

katrogan
Copy link
Contributor

@katrogan katrogan commented Jun 2, 2021

Signed-off-by: Katrina Rogan [email protected]

TL;DR

Add athena plugin

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

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

Signed-off-by: Katrina Rogan <[email protected]>

def get_custom(self, settings: SerializationSettings) -> Dict[str, Any]:
# This task is executed using the presto handler in the backend.
job = PrestoQuery(statement=self.query_template)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I stand corrected, I think it needs somethings set... they are just named differently... I think the minimum is the Database. We used Schema here to map it to...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep you're right! ty

@codecov
Copy link

codecov bot commented Jun 2, 2021

Codecov Report

Merging #504 (02bd714) into master (abd6f9f) will increase coverage by 0.02%.
The diff coverage is 98.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #504      +/-   ##
==========================================
+ Coverage   85.17%   85.19%   +0.02%     
==========================================
  Files         366      369       +3     
  Lines       27766    27823      +57     
  Branches     2263     2266       +3     
==========================================
+ Hits        23649    23705      +56     
  Misses       3496     3496              
- Partials      621      622       +1     
Impacted Files Coverage Δ
...ins/flytekit-athena/flytekitplugins/athena/task.py 95.83% <95.83%> (ø)
flytekit/models/presto.py 80.00% <100.00%> (ø)
...flytekit-athena/flytekitplugins/athena/__init__.py 100.00% <100.00%> (ø)
plugins/tests/athena/test_athena.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 abd6f9f...02bd714. Read the comment docs.

@katrogan
Copy link
Contributor Author

katrogan commented Jun 3, 2021

PTAL @kumare3 @EngHabu @wild-endeavor tested with flyteorg/flytesnacks#260

@katrogan katrogan changed the title Add athena plugin [still testing] Add athena plugin Jun 3, 2021
katrogan added 3 commits June 2, 2021 17:57
Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Katrina Rogan <[email protected]>
@wild-endeavor
Copy link
Contributor

wild-endeavor commented Jun 3, 2021

Let's change the top level folder name from athena to something else.

Because this exists https://pypi.org/project/athena/

So plugins/somethingelse/flytekitplugins/athena/
otherwise weird things happen if someone ever tries to import athena

Signed-off-by: Katrina Rogan <[email protected]>
@katrogan
Copy link
Contributor Author

katrogan commented Jun 3, 2021

@wild-endeavor thanks! done

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.

either way doesn't matter.


microlib_name = f"flytekitplugins-{PLUGIN_NAME}"

plugin_requires = ["flytekit>=0.18.0,<1.0.0"]
Copy link
Contributor

Choose a reason for hiding this comment

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

bump

katrogan and others added 3 commits June 3, 2021 11:11
Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Katrina Rogan <[email protected]>
@wild-endeavor
Copy link
Contributor

@kumare3 did you still have concerns about this PR? maybe we should add a test for a dynamic task wrapping an athena task?


def get_custom(self, settings: SerializationSettings) -> Dict[str, Any]:
# This task is executed using the presto handler in the backend.
job = PrestoQuery(statement=self.query_template, schema=self.task_config.database)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you not want to fill/expose workgroup and catalog too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@EngHabu done


def get_custom(self, settings: SerializationSettings) -> Dict[str, Any]:
# This task is executed using the presto handler in the backend.
job = PrestoQuery(
Copy link
Contributor

Choose a reason for hiding this comment

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

My concern is these settings @wild-endeavor. These are not standard, but I guess ok

@katrogan katrogan merged commit d3fb708 into master Jun 7, 2021
EngHabu pushed a commit that referenced this pull request Jun 25, 2021
Signed-off-by: Haytham Abuelfutuh <[email protected]>
wild-endeavor pushed a commit that referenced this pull request Jun 30, 2021
Signed-off-by: wild-endeavor <[email protected]>
wild-endeavor pushed a commit that referenced this pull request Jun 30, 2021
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: wild-endeavor <[email protected]>
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.

4 participants