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

chore: Make Feast UI importable as a NPM Module #2370

Merged
merged 10 commits into from
Mar 23, 2022

Conversation

tonyhschu
Copy link
Contributor

What this PR does / why we need it:
Makes the Feast UI importable as a NPM Module. This will make it easier for customers that want to extend Feast UI to have maintain their own repo while still consuming our changes.

Which issue(s) this PR fixes:

Fixes #

@codecov-commenter
Copy link

codecov-commenter commented Mar 5, 2022

Codecov Report

Merging #2370 (a58c59c) into master (c2389da) will decrease coverage by 28.34%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #2370       +/-   ##
===========================================
- Coverage   85.47%   57.12%   -28.35%     
===========================================
  Files         122      122               
  Lines       10714    10704       -10     
===========================================
- Hits         9158     6115     -3043     
- Misses       1556     4589     +3033     
Flag Coverage Δ
integrationtests ?
unittests 57.12% <ø> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../integration/online_store/test_universal_online.py 14.35% <0.00%> (-82.81%) ⬇️
.../integration/online_store/test_online_retrieval.py 17.39% <0.00%> (-82.61%) ⬇️
sdk/python/tests/utils/online_read_write_test.py 20.68% <0.00%> (-79.32%) ⬇️
...fline_store/test_universal_historical_retrieval.py 23.50% <0.00%> (-76.50%) ⬇️
sdk/python/feast/wait.py 23.52% <0.00%> (-70.59%) ⬇️
sdk/python/feast/infra/online_stores/redis.py 27.04% <0.00%> (-69.19%) ⬇️
...gration/registration/test_feature_service_apply.py 31.25% <0.00%> (-68.75%) ⬇️
sdk/python/tests/integration/e2e/test_usage_e2e.py 33.87% <0.00%> (-66.13%) ⬇️
sdk/python/tests/data/data_creator.py 34.78% <0.00%> (-65.22%) ⬇️
...dk/python/tests/integration/e2e/test_validation.py 35.18% <0.00%> (-64.82%) ⬇️
... and 66 more

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 c2389da...a58c59c. Read the comment docs.

@tonyhschu tonyhschu changed the title Make Feast UI importable feat: Make Feast UI importable as a NPM Module Mar 5, 2022
@tonyhschu tonyhschu force-pushed the tchu-feast-ui-as-npm-module branch 2 times, most recently from b5315b5 to a378d82 Compare March 6, 2022 22:33
ui/.gitignore Outdated Show resolved Hide resolved
ui/package.json Show resolved Hide resolved

interface FeaturesListProps {
featureViewName: string;
features: FeastFeatureColumnType[];
}

const FeaturesList = ({ featureViewName, features }: FeaturesListProps) => {
const { enabledFeatureStatistics } = useContext(FeatureFlagsContext);
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this intentional? seems not used so maybe can revert

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intentional. This enables the user to turn on this on as a prop on <FeastUI> i.e.

<FeastUI
  feastUIConfigs={{
    featureFlags: {
      enabledFeatureStatistics: true
    },
  }}
/>

This already works. I need to document this better hahaha.

@@ -0,0 +1,10 @@
import React from "react";

interface FeatureFlags {
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this let users flip it on / off in the top level FeastUI component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See this comment: #2370 (comment)

ui/src/contexts/FeatureFlagsContext.ts Show resolved Hide resolved
ui/src/pages/Layout.tsx Outdated Show resolved Hide resolved
ui/src/pages/Layout.tsx Outdated Show resolved Hide resolved
ui/src/FeastUISansProviders.tsx Show resolved Hide resolved
ui/src/FeastUISansProviders.tsx Show resolved Hide resolved
@tonyhschu tonyhschu marked this pull request as ready for review March 13, 2022 00:34
@tonyhschu tonyhschu requested a review from a team as a code owner March 13, 2022 00:34
@tonyhschu tonyhschu requested review from mavysavydav and removed request for a team March 13, 2022 00:34
@woop
Copy link
Member

woop commented Mar 13, 2022

Is this really a "feature"? Seems more like a chore or housekeeping. Bit of a gray area.

@tonyhschu
Copy link
Contributor Author

I didn't know what to pick. Happy to go with whatever @adchia thinks is more appropriate.

@adchia
Copy link
Collaborator

adchia commented Mar 14, 2022

Couple things to fix:

  • Make the version 0.19.4 (our next patch release) + publish this
  • In a followup PR, add documentation about publishing this package (add me to the feast org too)

@adchia
Copy link
Collaborator

adchia commented Mar 14, 2022

Also sign the commits

@adchia
Copy link
Collaborator

adchia commented Mar 14, 2022

Also: revert the deleting of the App.test.tsx

@adchia
Copy link
Collaborator

adchia commented Mar 14, 2022

convert it from feat: -> housekeeping:

@woop
Copy link
Member

woop commented Mar 14, 2022

  • Make the version 0.19.4 (our next patch release) + publish this

Any reason we can't do the following instead

  1. Make the packages.json version the same as other versions on master (0.19.0) and merge this PR in
  2. Add packages.json to https://github.com/feast-dev/feast/blob/master/infra/scripts/release/files_to_bump.txt
  3. Backport both (1) and (2) to v0.19-branch
  4. Change the version in packages.json on the v0.19-branch to 0.19.3
  5. Cut release on v0.19-branch, which will automatically bump packages.json to 0.19.4 before publishing
  6. (eventually) Cut release on master, which will automatically bump packages.json to 0.20.0 before publishing

@woop
Copy link
Member

woop commented Mar 21, 2022

@adchia @tonyhschu next steps here?

@tonyhschu
Copy link
Contributor Author

Hey @woop this is on me. I can follow the scheme you laid out above. I owe @adchia a couple other small changes - eg documenting how to publish to NPM.

@tonyhschu tonyhschu changed the title feat: Make Feast UI importable as a NPM Module housekeeping: Make Feast UI importable as a NPM Module Mar 22, 2022
Signed-off-by: Tony Chu <tonyhschu@gmail.com>
Signed-off-by: Tony Chu <tonyhschu@gmail.com>
Signed-off-by: Tony Chu <tonyhschu@gmail.com>
Signed-off-by: Tony Chu <tonyhschu@gmail.com>
Signed-off-by: Tony Chu <tonyhschu@gmail.com>
Signed-off-by: Tony Chu <tonyhschu@gmail.com>
Signed-off-by: Tony Chu <tonyhschu@gmail.com>
Signed-off-by: Tony Chu <tonyhschu@gmail.com>
Signed-off-by: Tony Chu <tonyhschu@gmail.com>
Signed-off-by: Tony Chu <tonyhschu@gmail.com>
@tonyhschu tonyhschu changed the title housekeeping: Make Feast UI importable as a NPM Module chore: Make Feast UI importable as a NPM Module Mar 22, 2022
Copy link
Collaborator

@adchia adchia left a comment

Choose a reason for hiding this comment

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

/lgtm

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adchia, tonyhschu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@feast-ci-bot feast-ci-bot merged commit 45db6dc into feast-dev:master Mar 23, 2022
@adchia adchia linked an issue Apr 21, 2022 that may be closed by this pull request
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feast Web UI
6 participants