Feast 0.3 Continuous Integration (CI) Update #271
Conversation
For those commands that pipe the test output
- Newer version is hosted on Maven central which is more maintained than other repo
Otherwise it fails to run some tests due to insufficient memory
- Set project to empty (DataflowOptions in Beam require project to be not null) - Ignore error when shutting down Kafka server during tear down. This should not affect test result.
- This tar archive contains packages used by Feast 0.3 rather than previous version of Feast
- Allow more ranges of versions as long as they are not breaking changes
…source Update in progress script for integration test: TODO.sh
- float64 should map to ValueType.DOUBLE instead of ValueType.FLOAT - Ensure Python SDK installation use protobuf v3.10 from pypi. Earlier version of protobuf do not support accessing Enum value in Proto object directly. - Ensure version is passed as INT in e2e test. Set default allow_dirty to true so it's easier to debug error when running e2e test locally.
|
Created an issue to track these updated #275 |
…cript - Since there are only 6 tests, there is not much benefit of combining tests into a single script Having each test defined on diff script file makes debugging easier
- Update test case DirectRunnerJobManagerTest.shouldStartDirectJobAndRegisterPipelineResult to set project option to empty string for DirectRunner - Update /usr/sbin/policy-rc.d used in Maven Docker image, so Redis installation will succeed
- Also use cached maven packages when running e2e test
… in feast e2e test
davidheryanto
requested review from
pradithya,
thirteen37,
tims,
woop and
zhilingc
as code owners
| - name: GOOGLE_APPLICATION_CREDENTIALS | ||
| value: /etc/service-account/service-account.json | ||
| command: [".prow/scripts/run_unit_test.sh", "--component", "core"] | ||
| command: [".prow/scripts/test-core-ingestion.sh"] |
There was a problem hiding this comment.
@davidheryanto Would it make more sense to move these scripts to /tests or /infra instead of prow?
| " | ||
| # Start Feast Core in background | ||
| cat <<EOF > /tmp/core.application.yml | ||
| grpc: |
There was a problem hiding this comment.
Do we want to leave this yml file here? It seems like it will go stale eventually, but I guess it is ok as a start.
There was a problem hiding this comment.
Yes we can move the test scripts and the resources (like yaml configuration) into tests folder or infra folder as an improvement
| cd tests/e2e | ||
|
|
||
| set +e | ||
| pytest --junitxml=${LOGS_ARTIFACT_PATH}/python-sdk-test-report.xml |
There was a problem hiding this comment.
I think we need to make pytest more verbose so that we can see which tests have passed. Perhaps this would help: http://doc.pytest.org/en/latest/usage.html#detailed-summary-report
There was a problem hiding this comment.
Yeah sounds good
| --archive-uri gs://feast-templocation-kf-feast/.m2.2019-10-24.tar \ | ||
| --output-dir /root/ | ||
|
|
||
| # Skip Maven enforcer: https://stackoverflow.com/questions/50647223/maven-enforcer-issue-when-running-from-reactor-level |
There was a problem hiding this comment.
@ches Do you have any thoughts on this? I'm not well versed enough with Maven or the enforcer plugin to comment.
There was a problem hiding this comment.
Hmm, I hadn't encountered this but I can reproduce it with mvn --projects serving test.
However a solution rather than skipping Enforcer seems to be to include --also-make, that works for me (short version for day-to-day typing: -am).
That isn't normally strictly necessary since serving doesn't have any inter-module dependency (like core does on ingestion), but it seems like specifying it makes it run as a Reactor build that includes serving and the parent POM together, so that their relationship can be verified.
--also-make is often what you want in multi-module projects. @smadarasmi had a related question about running components after the dependency management changes that prompted me to make additions to development notes, I should have a PR with those today.
There was a problem hiding this comment.
Also, I'm not sure the install step is really needed here FWIW, just test should suffice for unit tests.
There was a problem hiding this comment.
I've tried making the change to --also-make in this Prow script in #276. Not sure if the CI build is functional yet, but maybe I can learn how to try it.
There was a problem hiding this comment.
Yeah we can try those options.
I added the install script before test to follow what Travis CI does for Maven project
https://docs.travis-ci.com/user/languages/java/#maven-dependency-management
But I guess we can also omit it also because it makes it simpler.
Currently the test worflow is derived from config.yaml in .prow folder. Updating config.yaml is not that straightforward because the config_updater plugin in Prow only updates the config.yaml at a certain interval after merge to master. So I currently update the config.yaml manually.
But if you update say test-serving.sh, the CI should use the updated test-serving.sh
| parser.addoption("--serving_url", action="store", default="localhost:6565") | ||
| parser.addoption("--allow_dirty", action="store", default="false") | ||
| parser.addoption("--serving_url", action="store", default="localhost:6566") | ||
| parser.addoption("--allow_dirty", action="store", default="true") |
There was a problem hiding this comment.
Are you sure about making the default true here? I made it false so that when people run e2e tests locally that it wouldnt destroy a real cluster.
There was a problem hiding this comment.
Yeah sounds good. I think i'll change the test script to specify allow_dirty options explicitly rather than changing the default
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: davidheryanto, 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 |
|
/lgtm |
|
Merging this since it's important, but we can continue to discuss the comments above. |
| </requireMavenVersion> | ||
| <requireJavaVersion> | ||
| <version>1.8.0</version> | ||
| <version>[1.8,1.9)</version> |
There was a problem hiding this comment.
It's a >= constraint by default—a range makes sense for Maven version just in case of breaking changes, but for Java IMO it would be good to build 1.8 and JDK 11 in CI so we might need something fancier here.
There was a problem hiding this comment.
Currently I limit it to Java 8 because we're using Beam SDK and I believe it's not fully compatible with newer major versions of Java.
https://issues.apache.org/jira/browse/BEAM-2530
We can support more versions once we're sure it's compatible
…oject level (#276) * Fix using spring-boot:run from project root A little fix when some subproject modules are Spring Boot apps, but the parent is not. Also document how to overcome IntelliJ CE woes with Spring Boot, and inter-module Maven dependencies. * Get Spring managed dep versions via their BOM Spring has done the work of finding direct and transitive versions of dependencies compatible with their ecosystem, and declaring them as a BOM. It's in our best interest to heed it as much as we can. * ci: Try --also-make instead of skipping Enforcer Per discussion: #271 (comment) The `install` step should not be necessary for the unit tests, but we should use `--batch-mode` on the test step for CI. * Remove maven-shade-plugin from core It was resulting in a non-functional JAR, appears to be incomplete configuration. It seems unlikely to me that core will be used as a library, if there is a need the shading configuration can be fixed but it probably needs other considerations anyway.
* Update prow config and test script so it works for Feast 0.3 * Update CI test script for golang to use correct path * Set pipefail option so test exit code is correct For those commands that pipe the test output * Update group id and version grpc-spring-boot-starter dependency - Newer version is hosted on Maven central which is more maintained than other repo * Add JVM heap settings for Maven surefire Otherwise it fails to run some tests due to insufficient memory * Fix potential setup/teardown error when running ImportJobTest - Set project to empty (DataflowOptions in Beam require project to be not null) - Ignore error when shutting down Kafka server during tear down. This should not affect test result. * Update remote URI to download cached Maven packages - This tar archive contains packages used by Feast 0.3 rather than previous version of Feast * Fix path to logs artifacts * Update maven enforcer rule for Maven and JDK versions - Allow more ranges of versions as long as they are not breaking changes * Use batch mode when initializing Maven for cleaner build log * Skip maven enforcer when running test for specific project * Fix incorrect order for java-sdk test script * Install database and kafka dependencies for Feast end to end test * Update Python SDK to not set source in FeatureSet when using default source Update in progress script for integration test: TODO.sh * Fix Python SDK type mapping - float64 should map to ValueType.DOUBLE instead of ValueType.FLOAT - Ensure Python SDK installation use protobuf v3.10 from pypi. Earlier version of protobuf do not support accessing Enum value in Proto object directly. - Ensure version is passed as INT in e2e test. Set default allow_dirty to true so it's easier to debug error when running e2e test locally. * Refactor prow scripts such that every test is defined in a separate script - Since there are only 6 tests, there is not much benefit of combining tests into a single script Having each test defined on diff script file makes debugging easier * Remove reference to actual project id * Log current stage in end to end test script - Update test case DirectRunnerJobManagerTest.shouldStartDirectJobAndRegisterPipelineResult to set project option to empty string for DirectRunner - Update /usr/sbin/policy-rc.d used in Maven Docker image, so Redis installation will succeed * Fix missing option to specify miniconda download target - Also use cached maven packages when running e2e test * Fix incorrect path to installing Python SDK * Add small wait after ingestion, before validating the saved features, in feast e2e test * Increase wait time before checking, after applying feature set * Make log cleaner
* Map jobs source::sink instead of fs::sink. Allow users to specify topics. Provision single topic by default. * Add validation for kafka source * Remove feature stream service and instantiate it at initialisation * Update end-to-end test config * Add DefaultSource bean * Feast 0.3 Continuous Integration (CI) Update (#271) * Update prow config and test script so it works for Feast 0.3 * Update CI test script for golang to use correct path * Set pipefail option so test exit code is correct For those commands that pipe the test output * Update group id and version grpc-spring-boot-starter dependency - Newer version is hosted on Maven central which is more maintained than other repo * Add JVM heap settings for Maven surefire Otherwise it fails to run some tests due to insufficient memory * Fix potential setup/teardown error when running ImportJobTest - Set project to empty (DataflowOptions in Beam require project to be not null) - Ignore error when shutting down Kafka server during tear down. This should not affect test result. * Update remote URI to download cached Maven packages - This tar archive contains packages used by Feast 0.3 rather than previous version of Feast * Fix path to logs artifacts * Update maven enforcer rule for Maven and JDK versions - Allow more ranges of versions as long as they are not breaking changes * Use batch mode when initializing Maven for cleaner build log * Skip maven enforcer when running test for specific project * Fix incorrect order for java-sdk test script * Install database and kafka dependencies for Feast end to end test * Update Python SDK to not set source in FeatureSet when using default source Update in progress script for integration test: TODO.sh * Fix Python SDK type mapping - float64 should map to ValueType.DOUBLE instead of ValueType.FLOAT - Ensure Python SDK installation use protobuf v3.10 from pypi. Earlier version of protobuf do not support accessing Enum value in Proto object directly. - Ensure version is passed as INT in e2e test. Set default allow_dirty to true so it's easier to debug error when running e2e test locally. * Refactor prow scripts such that every test is defined in a separate script - Since there are only 6 tests, there is not much benefit of combining tests into a single script Having each test defined on diff script file makes debugging easier * Remove reference to actual project id * Log current stage in end to end test script - Update test case DirectRunnerJobManagerTest.shouldStartDirectJobAndRegisterPipelineResult to set project option to empty string for DirectRunner - Update /usr/sbin/policy-rc.d used in Maven Docker image, so Redis installation will succeed * Fix missing option to specify miniconda download target - Also use cached maven packages when running e2e test * Fix incorrect path to installing Python SDK * Add small wait after ingestion, before validating the saved features, in feast e2e test * Increase wait time before checking, after applying feature set * Make log cleaner * Map jobs source::sink instead of fs::sink. Allow users to specify topics. Provision single topic by default. * Add validation for kafka source * Remove feature stream service and instantiate it at initialisation * Update end-to-end test config * Add DefaultSource bean * Change column name to match param name
Current CI script using Prow is working correctly only for Feast 0.1.
In Feast 0.3, there are updates to the dependencies and steps required to build and test different Feast components.
This is a pull request that update the test and build scripts for Feast 0.3.
Resolves #275
The CI stages performed are the following
(check
.prow/scripts/folder and.prow/config.yamlfor details):NOTE:
This is likely to cause current tests in master to fail. This is expected.