v0.3 backport: Add feature and feature set labels #737
Conversation
ches
requested review from
davidheryanto,
pradithya,
thirteen37,
woop and
zhilingc
as code owners
| List<FeatureSpec> appliedFeatureSpecs = | ||
| new ArrayList<>(appliedFeatureSetSpec.getFeaturesList()); | ||
| appliedFeatureSpecs.sort(Comparator.comparing(FeatureSpec::getName)); | ||
|
|
||
| List<Map<String, String>> featureSpecsLabels = | ||
| featureSpecs.stream().map(e -> e.getLabelsMap()).collect(Collectors.toList()); | ||
| assertThat(appliedEntitySpecs, equalTo(entitySpecs)); | ||
| assertThat(appliedFeatureSpecs, equalTo(featureSpecs)); | ||
| assertThat(featureSpecsLabels, equalTo(featureLabels)); |
There was a problem hiding this comment.
I just noticed this when raising this PR: I believe that the featureSpecsLabels here should be assigned from appliedFeatureSpecs.stream() instead of featureSpecs.stream(), otherwise we are asserting on test setup code instead of covering actual roundtrip through the feature set being applied. I've asked @suwik to double-check my reasoning.
I've made that change locally and the test passes, so I believe it proves there is no functional issue with feature-level labels, we just aren't covering what we want to with the test. I'll include the update in this PR if @suwik corroborates, but the same change should be forward-ported to master, so I want to track that.
Backports feast-dev#536 to v0.3
ches
force-pushed
the
v0.3-backport-536-labels
branch
from
ec8cfe2 to
2a9d672
Compare
It appears that this test was only exercising the test setup code, not covering that labels work when applied. This change should be forward-ported to master.
|
😄 I went ahead and pushed the change that I think makes sense, so we can forward-port the commit. I'm happy to go ahead with it so I can get a few more things done by Monday toward an 0.3.8 release we can use. |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ches, woop 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 |
What this PR does / why we need it:
Backports #536 to v0.3, courtesy of @suwik's work.
We don't typically backport features, but I'm proposing such in this case for practical reasons:
datatypes-javawith the labels fields added to the protos. Our Feast SDK is hosted on github.com pending being open sourced, and its GitHub Actions CI build cannot reach our behind-the-firewall Artifactory to resolve the private dependency version. We will live on v0.3 a little while longer, so I'd like to not have to relegate "v0.3 with labels" to an SDK branch where CI isn't usable. We could alternatively publish the datatypes to Maven Central under our own group ID, but it's rather roundabout, so I wanted to try reception in mainline Feast before turning to that.This is not just housekeeping, but my note about maintenance on #734 stands.
The largest contributor to the size of this PR is Python code generated from protobufs. In v0.3 the generated code is still tracked in Git, and it seems like it was stale before this change, because running the codegen produced a lot more changes than just the label additions.
Does this PR introduce a user-facing change?: