feat: generalize application model (+ add databricks application support) #1398

jroof88 · 2021-07-30T04:54:05Z

Summary of Changes

Currently the Application model isn't very usable to anyone who doesn't use Airflow. This commit generalize the model while also freezing current Airflow logic into place for backwards compatibility. I update docs and tests as well to make sure it works properly.

I also add a databricks logo for rendering databricks applications nicely on the frontend :)

Tests

I refactored the tests in test_application.py to allow for multiple test cases and added proper testing for the databricks model type

Documentation

I updated the model documentation to remove the concept Airflow only support

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.
PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.

mgorsk1 · 2021-08-04T05:56:59Z

Do you think there is a way to refactor this class so no coding is required to introduce another app?

I think only thing that would need to be done is figuring out how to render keys but apart from that it would be much user friendly and scalable.

Btw shall we introduce new icon for databricks app?

jroof88 · 2021-08-04T16:53:43Z

Do you think there is a way to refactor this class so no coding is required to introduce another app?

I think only thing that would need to be done is figuring out how to render keys but apart from that it would be much user friendly and scalable.

Btw shall we introduce new icon for databricks app?

@mgorsk1 Yeah so this was kind of my dilemma while doing this. I did introduce a new logo for Databricks in this PR (HERE). I don't think it would be very hard to make this open to any kind of app but then the UI rendering would just show up with no icon? Is that ok? Are there any other instances of this happening? I think what I would do is leave all of the Airflow specific things for backwards compatibility and then have a new template for any other type of app.

I would definitely vote for this route as long as folks are ok with developer essentially being able to put in any kind of app. We could also maybe log a warning message or something in their app doesn't have an icon.

mgorsk1 · 2021-08-04T17:14:25Z

We can have a default icon for when the app doesn't match supported ones. The same happens for tables if the database name doesn't have matching icon.

+1 for making generic design for anything except Airflow.

jroof88 · 2021-08-04T17:55:20Z

We can have a default icon for when the app doesn't match supported ones. The same happens for tables if the database name doesn't have matching icon.

@mgorsk1 Cool this is super helpful. I did not know this. I will refactor this PR to be generic but freeze the Airflow constructs already in place for backwards compatibility 👍. Will reach out for a review when it is ready.

feng-tao · 2021-08-05T01:40:42Z

make sense to make the application more generic to support other workflow type including databricks job/prefect etc.

@jroof88 I am curious how you link the databricks job with the table. I did something similar for databricks which requires some internal usage tracking(not visible external).

jroof88 · 2021-08-05T23:42:26Z

make sense to make the application more generic to support other workflow type including databricks job/prefect etc.

@jroof88 I am curious how you link the databricks job with the table. I did something similar for databricks which requires some internal usage tracking(not visible external).

@feng-tao My plan is to link the amundsen table to the databricks ETL job that writes to the table so it's is easy for folks to go from table to job without have to naviagate/search the databricks jobs ui

feng-tao · 2021-08-06T00:28:36Z

Is it through manual mapping?

jroof88 · 2021-08-06T00:47:03Z

Is it through manual mapping?

@feng-tao We have code that parses our databricks jobs via the api and then can map the links to specific tables yes

jroof88 · 2021-08-06T00:47:15Z

@feng-tao @mgorsk1 This is ready for a review!

jroof88 · 2021-08-09T17:08:17Z

Hi @feng-tao & @mgorsk1 friendly ping for a review here! All refactored to be generalized while also adding the dbx logo for folks to use databricks as the application type

mgorsk1

one nit but lgtm

mgorsk1 · 2021-08-09T18:51:59Z

databuilder/databuilder/models/application.py

+
+        # The Application model was originally designed to only be compatible with Airflow
+        # If the type is Airflow we must use the hardcoded Airflow constants for backwards compatibility
+        if self.application_type == 'Airflow':


just to be 200% sure can we do if self.application_type.lower() == 'airflow' ?

Yup! Done ✅

Currently the Application model isn't very usable to anyone who doesn't use Airflow. This commit generalize the model while also freezing current Airflow logic into place for backwards compatibility. I update docs and tests as well to make sure it works properly. Signed-off-by: jroof88 <jack.roof@samsara.com>

Signed-off-by: jroof88 <jack.roof@samsara.com>

Bump databuilder minor version to pick up new application changes Signed-off-by: jroof88 <jack.roof@samsara.com>

Bump frontend minor version to pick up new databricks application logo rendering Signed-off-by: jroof88 <jack.roof@samsara.com>

In amundsen-io#1398 we generalized the Application model to be any application not just airflow. In that PR, I removed a arguement to the init for the model. This broke the sample data loader but it could also break peoples workflows who are already using Application model. This commit brings the arguement back.

In amundsen-io#1398 we generalized the Application model to be any application not just airflow. In that PR, I removed a arguement to the init for the model. This broke the sample data loader but it could also break peoples workflows who are already using Application model. This commit brings the arguement back. Signed-off-by: jroof88 <jack.roof@samsara.com>

…ort) (amundsen-io#1398) Signed-off-by: jroof88 <jack.roof@samsara.com>

jroof88 requested review from allisonsuarez, dorianj, feng-tao, Golodhros, mgorsk1, sewardgw, ttannis, verdan and a team as code owners July 30, 2021 04:54

boring-cyborg bot added area:databuilder From databuilder folder area:frontend From the Frontend folder category:models labels Jul 30, 2021

jroof88 force-pushed the jroof-amundsen-databricksApplication branch from 23f8589 to 849a7e6 Compare July 30, 2021 21:35

boring-cyborg bot added the area:docs label Jul 30, 2021

jroof88 force-pushed the jroof-amundsen-databricksApplication branch from 849a7e6 to 05328ae Compare July 30, 2021 21:36

jroof88 changed the title ~~WIP for Databricks Application support~~ feat: support databricks application type Jul 30, 2021

jroof88 force-pushed the jroof-amundsen-databricksApplication branch 5 times, most recently from 4e0cf63 to 7cd8f04 Compare July 30, 2021 23:43

jroof88 force-pushed the jroof-amundsen-databricksApplication branch from 7cd8f04 to 98cca58 Compare August 6, 2021 00:45

jroof88 requested review from dechoma, jinhyukchang and youngyjd as code owners August 6, 2021 00:45

jroof88 force-pushed the jroof-amundsen-databricksApplication branch 4 times, most recently from e18a212 to 9245807 Compare August 6, 2021 01:03

mgorsk1 approved these changes Aug 9, 2021

View reviewed changes

jroof88 added 4 commits August 9, 2021 20:30

feat: add databricks application logo for frontend render

108eb95

Signed-off-by: jroof88 <jack.roof@samsara.com>

chore: bump databuilder version

c4d79e3

Bump databuilder minor version to pick up new application changes Signed-off-by: jroof88 <jack.roof@samsara.com>

chore: bump frontend version

15b0d4a

Bump frontend minor version to pick up new databricks application logo rendering Signed-off-by: jroof88 <jack.roof@samsara.com>

jroof88 force-pushed the jroof-amundsen-databricksApplication branch from 9245807 to 15b0d4a Compare August 10, 2021 03:30

jroof88 requested a review from mgorsk1 August 10, 2021 03:30

jroof88 changed the title ~~feat: support databricks application type~~ feat: generalize application model (+ add databricks application support) Aug 11, 2021

mgorsk1 merged commit 1a75a2f into amundsen-io:main Aug 12, 2021

jroof88 mentioned this pull request Aug 24, 2021

fix: Application model backwards compatibility #1456

Merged

2 tasks

This was referenced Nov 6, 2021

feat: allow multiple applications #1569

Merged

Feat: Further generalize applications #1577

Merged

zacr pushed a commit to SaltIO/amundsen that referenced this pull request May 13, 2022

feat: generalize application model (+ add databricks application supp…

6871a35

…ort) (amundsen-io#1398) Signed-off-by: jroof88 <jack.roof@samsara.com>

hansadriaans pushed a commit to DataChefHQ/amundsen that referenced this pull request Jun 30, 2022

feat: generalize application model (+ add databricks application supp…

7386702

…ort) (amundsen-io#1398) Signed-off-by: jroof88 <jack.roof@samsara.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: generalize application model (+ add databricks application support) #1398

feat: generalize application model (+ add databricks application support) #1398

jroof88 commented Jul 30, 2021 •

edited

Loading

mgorsk1 commented Aug 4, 2021 •

edited

Loading

jroof88 commented Aug 4, 2021

mgorsk1 commented Aug 4, 2021

jroof88 commented Aug 4, 2021

feng-tao commented Aug 5, 2021

jroof88 commented Aug 5, 2021

feng-tao commented Aug 6, 2021

jroof88 commented Aug 6, 2021 •

edited

Loading

jroof88 commented Aug 6, 2021

jroof88 commented Aug 9, 2021 •

edited

Loading

mgorsk1 left a comment

mgorsk1 Aug 9, 2021

jroof88 Aug 10, 2021

feat: generalize application model (+ add databricks application support) #1398

feat: generalize application model (+ add databricks application support) #1398

Conversation

jroof88 commented Jul 30, 2021 • edited Loading

Summary of Changes

Tests

Documentation

CheckList

mgorsk1 commented Aug 4, 2021 • edited Loading

jroof88 commented Aug 4, 2021

mgorsk1 commented Aug 4, 2021

jroof88 commented Aug 4, 2021

feng-tao commented Aug 5, 2021

jroof88 commented Aug 5, 2021

feng-tao commented Aug 6, 2021

jroof88 commented Aug 6, 2021 • edited Loading

jroof88 commented Aug 6, 2021

jroof88 commented Aug 9, 2021 • edited Loading

mgorsk1 left a comment

Choose a reason for hiding this comment

mgorsk1 Aug 9, 2021

Choose a reason for hiding this comment

jroof88 Aug 10, 2021

Choose a reason for hiding this comment

jroof88 commented Jul 30, 2021 •

edited

Loading

mgorsk1 commented Aug 4, 2021 •

edited

Loading

jroof88 commented Aug 6, 2021 •

edited

Loading

jroof88 commented Aug 9, 2021 •

edited

Loading