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

feat(ml): show custom properties for MLFeatureTable in UI #4706

Conversation

maaaikoool
Copy link
Contributor

Adds the properties tab to MLFeatureTable, which is quite handy to add custom metadata that does not fit with the actual model.

image

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions
Copy link

github-actions bot commented Apr 20, 2022

Unit Test Results (build & test)

  97 files  ±0    97 suites  ±0   22m 47s ⏱️ + 2m 35s
701 tests ±0  642 ✔️ ±0  59 💤 ±0  0 ±0 

Results for commit 51f1be2. ± Comparison against base commit f659cc8.

♻️ This comment has been updated with latest results.

@jjoyce0510 jjoyce0510 requested a review from gabe-lyons April 20, 2022 15:43
Copy link
Contributor

@gabe-lyons gabe-lyons left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for this.

@@ -41,6 +42,8 @@ public MLFeatureTableProperties apply(@NonNull final com.linkedin.ml.metadata.ML
.collect(Collectors.toList()));
}

result.setCustomProperties(StringMapMapper.map(mlFeatureTableProperties.getCustomProperties()));
Copy link
Contributor

Choose a reason for hiding this comment

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

@maaaikoool I think we need a (if mlFeatureTableProperties. getCustomProperties() != null) check here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that customProperties is non nullable ( it has a @Nonnull annotation )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@maaaikoool maaaikoool Apr 20, 2022

Choose a reason for hiding this comment

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

I've double checked with other mappers and it seems that indeed we are using a conditional assignment checking for hasCustomProperties so I've added it for consistency 👌

Copy link
Contributor

@gabe-lyons gabe-lyons left a comment

Choose a reason for hiding this comment

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

thanks!

@gabe-lyons gabe-lyons merged commit 788fb8f into datahub-project:master Apr 21, 2022
@maaaikoool
Copy link
Contributor Author

thanks for the review!

maggiehays pushed a commit to maggiehays/datahub that referenced this pull request Aug 1, 2022
…oject#4706)

* feat(ml): show custom properties for MLFeatureTable in UI

* Make assignment conditional

* Fix lint
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.

2 participants