Backport Sonatype publishing and datatypes module for v0.3.x

[ches]

Backports #394, #406, and #391

[feast-ci-bot]

Hi @ches. Thanks for your PR.

I'm waiting for a gojek member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[woop]

/ok-to-test

[woop]

@ches these flaky tests are driving me crazy. Meeting with the team this week to see if we can find the root cause.

[davidheryanto]

I think this pull request makes the javadoc-plugin runs on every build hence errors in our Javadoc will cause Maven build and CI to fail.

I think the current failure is due to these lines:

https://github.com/ches/feast/blob/36ea2394454f5dede1087c31ec640cea38675ed6/core/src/main/java/feast/core/service/SpecService.java#L144
We should encode <, > characters in Javadoc as < and > respectively

https://github.com/ches/feast/blob/36ea2394454f5dede1087c31ec640cea38675ed6/core/src/main/java/feast/core/service/SpecService.java#L85
@param GetFeatureSetRequest should be @param request

I found that by looking at full log output

[woop]

@davidheryanto how do we bubble up these problems through the logs without having to click on the full build log? Is the exit code being ignored at the build stage?

[ches]

Thanks @davidheryanto for digging out the cause.

I think this pull request makes the javadoc-plugin runs on every build hence errors in our Javadoc will cause Maven build and CI to fail.

Indeed #394 added the Javadoc plugin bound to the jar goal, so when end-to-end tests run mvn package that is triggering. That PR also fixed the doc formatting issues but some of the docs changed completely with functional changes on master, so I excluded them from the cherry-pick.

An end-to-end test phase is a frustrating time to fail on malformatted Javadoc… (and generating them is a bit of a waste of time outside of release preparation). Perhaps we can do something like:

1. For tests, run mvn package with -Dmaven.javadoc.skip
2. Worry about Javadoc cleanup at release preparation time, automated in a release build

Or if you'd like for contributors to maintain error-free Javadoc proactively:

1. Run some type of Coding Standards check early in the build steps, including invoking a goal from the Javadoc plugin
2. For tests, run mvn package with -Dmaven.javadoc.skip

Javadoc skip could be enabled by default in the root build, and disabled in a release profile.

(Aside: I've noticed Spotless formatting check isn't happening currently, because it defaults to verify phase and CI doesn't go that far—been meaning to raise this elsewhere, it could fall under such a fail-fast standards check).

Let's take that discussion to a separate issue, I'll fix the straggler HTML encoding issues here momentarily to get CI green for this one.

[ches]

/retest

[ches]

Investigating the failure will take me some time—I don't want to shirk that responsibility, but before I spend it can anyone quickly recognize it as something that has been fixed on master that I should maybe pull in? Because it seems unlikely to me that it's a regression introduced by this branch, GitHub doesn't seem to show that status checks passed on the latest commit on v0.3-branch.

To save you a click, it's a single failure in the bq-batch-retrieval.py suite, the test_get_batch_features_with_file case:

>   assert output["entity_id"].to_list() == [int(i) for i in output["file_feature_set_v1_feature_value"].to_list()]
E   TypeError: int() argument must be a string, a bytes-like object or a number, not 'NoneType'

bq-batch-retrieval.py:90: TypeError

The print statement of the dataframe in question shows:

  file_feature_set_v1_feature_value  
0                              None  
1                              None  
2                              None  
3                              None  
4                              None  

[davidheryanto]

I think it is due to missing wait after running Feast apply.
I hope this will fix that.
#412

[davidheryanto]

/retest

[ches]

I'll bring #406 into this PR as we discussed there, but I would really like to get #391 closed first.

[woop]

Thanks @davidheryanto for digging out the cause.

I think this pull request makes the javadoc-plugin runs on every build hence errors in our Javadoc will cause Maven build and CI to fail.

Indeed #394 added the Javadoc plugin bound to the jar goal, so when end-to-end tests run mvn package that is triggering. That PR also fixed the doc formatting issues but some of the docs changed completely with functional changes on master, so I excluded them from the cherry-pick.

An end-to-end test phase is a frustrating time to fail on malformatted Javadoc… (and generating them is a bit of a waste of time outside of release preparation). Perhaps we can do something like:

1. For tests, run mvn package with -Dmaven.javadoc.skip
2. Worry about Javadoc cleanup at release preparation time, automated in a release build

Or if you'd like for contributors to maintain error-free Javadoc proactively:

1. Run some type of Coding Standards check early in the build steps, including invoking a goal from the Javadoc plugin
2. For tests, run mvn package with -Dmaven.javadoc.skip

Javadoc skip could be enabled by default in the root build, and disabled in a release profile.

(Aside: I've noticed Spotless formatting check isn't happening currently, because it defaults to verify phase and CI doesn't go that far—been meaning to raise this elsewhere, it could fall under such a fail-fast standards check).

Let's take that discussion to a separate issue, I'll fix the straggler HTML encoding issues here momentarily to get CI green for this one.

I prefer the latter option so that we proactively maintain error-free docs.

Are we happy to move this to an issue?

[woop]

/lgtm
/approve

[feast-ci-bot]

[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:

• OWNERS [woop]

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

[ches]

Are we happy to move this to an issue?

Sorry I didn't update my comment, but I created one already at #408.

I will bring the changes from #406 into this branch as part of the backport, just wanting to get #391 through first. We can mark this PR on hold in the meantime.

[ches]

@davidheryanto I went with the bare minimum of augmenting the script here, since it makes sense that these are always published together by the same job, the conditions for when to run are the same, etc. Do you think anything more than this is needed, docs or otherwise?

We will need to forward-port this change to master, I forgot it in #391 since the release process came after I started.

[woop]

/lgtm

[woop]

Are we happy to move this to an issue?

Sorry I didn't update my comment, but I created one already at #408.

I will bring the changes from #406 into this branch as part of the backport, just wanting to get #391 through first. We can mark this PR on hold in the meantime.

Oh damn, I didn't see there were new comments here. I already did an /lgtm so the merge went ahead.

Can we use a separate PR for that?

[ches]

Oh damn, I didn't see there were new comments here. I already did an /lgtm so the merge went ahead.

Can we use a separate PR for that?

I already cherry-picked in the squash of #406 here so we're good with that. The only open part was my question to David just above from the last commit I tacked on. If we need any change for that I'll open a new PR.

[woop]

Oh damn, I didn't see there were new comments here. I already did an /lgtm so the merge went ahead.
Can we use a separate PR for that?

I already cherry-picked in the squash of #406 here so we're good with that. The only open part was my question to David just above from the last commit I tacked on. If we need any change for that I'll open a new PR.

Ok, in that case it should be fine I think!