Set default project on Python client - #532
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Joostrothweiler The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @Joostrothweiler. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
/assign @davidheryanto |
| Args: | ||
| feature_sets: List of feature sets that will be registered | ||
| """ | ||
| if self.project is not None and self.project not in self.list_projects(): |
There was a problem hiding this comment.
Although this technically work for the Python SDK, it won't cover the Feast Core side. Some of our users interface directly with the gRPC API. So the creation should ideally happen on that side.
There was a problem hiding this comment.
@woop agree that this should not be the responsibility of the client. Since I still need to update the tests to mock the list_projects response, do you see this as an appropriate solution for now?
Perhaps going forward, it would be more appropriate to create the default project at the moment of building Feast Core?
There was a problem hiding this comment.
I think it would be safer to just create a project in Feast Core whenever a new project name comes in. Moving the default project string to Feast Core later makes sense as well, but I am happy to leave that in the Python SDK for now.
|
This is awesome, thanks @Joostrothweiler! |
|
/ok-to-test |
|
@Joostrothweiler: The following test failed, say
DetailsInstructions 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. I understand the commands that are listed here. |
* Encode feature row before storing in Redis * Include encoding as part of RedisMutationDoFn Co-authored-by: Khor Shu Heng <khor.heng@gojek.com>
Please see this issue for more details feast-dev#527
…east-dev#534) This seems to make the test pass more deterministically If this value is higher than the one used for sending output (50ms) some messages may be lost leading to failed test
) * Update base Docker image for building Feast Serving image - Add clean phase before packaging for more deterministic build (in case host directory is dirty) - Move the downloading of grpc-health-probe in Feast Serving to build stage so the production stage does not need extra tools like wget, for slimmer production image. * Update base Docker image for Feast Serving in Dockerfile.dev
* Add Make commands for format, lint, flake8, spotless, isort, black, and refactor * Add mypy test * Add lint tests to CI * Fix broken Python test * Fix broken test for Python * Add black to dependencies * Remove Python Protos * Add automatic local linting * Update precommit names * Add black exclusions * Add tensorflow metadata proto generation * Ignore tf meta directory * Add build essentials to install make in CI * Add exports back to __init__.py * Add __all__ to export * Add white space to export * Add source to export * Fix python export formatting
* Create new configuration class * Add new configuration class to CLI and Client class * Add new configuration class to CLI and Client class * Add project key to config in client
* Add unit tests to GitHub Actions * Fix linting dependencies * Fix linting dependencies command * Remove GOROOT variable from Makefile * Remove batch retrieval test and fix golang dependencies * Change .prow folder to infra to fix tests * Downgrade protoc version
* Add pollingInterval config option * Add missing prefix * Increase polling interval
* Changed Runner.getName() to Runner.toString() when passing to Job.runner in DataflowJobManager This is necessary to standardise the use of Runner.toString() when passing to the Job.runner, so that code dependending on Job.runner would know what to expect. * Document how & when Runner.toString() or Runner.getName() should be used * Convert getName() to toString(). Use name() for Job.runner. Use toString() to render human readable strings while using the non overriding name() for code dependencies. Co-authored-by: Zhu Zhanyan <zhu.zhanyan@gojek.com>
feast-dev#578) Bug caused by toString() method being used in the conversion lambda instead of the getName() method. Since lambda duplicates functionality already implemented by Feature.fromProto, removing the lambda block in favor using the method to do the conversion. Co-authored-by: Zhu Zhanyan <zhu.zhanyan@gojek.com>
Co-authored-by: Khor Shu Heng <khor.heng@gojek.com>
* Add badges and rename linting to code standards * Fix code standards workflow link
* Add Telco Churn Tutorial * Remove telco_customer_churn.csv
|
@Joostrothweiler I've extended the Python SDK's configuration management. You can now see default options here https://github.com/gojek/feast/blob/master/sdk/python/feast/constants.py#L40 In fact I have already set the default project there. So basically we should always read the project from either the provided object or the configuration object (there are helper methods). We shouldnt have to call Feast Core to create the project before apply. That can all be handled in the apply step. Let me know if you want to refactor (and rebase) your PR, otherwise I can pick it up. |
We are getting cache misses between builds. This can be seen here: https://github.com/gojek/feast/runs/541172041?check_suite_focus=true This is a common problem with GitHub Actions, and apparently its necessary to change the cache key in order to resolve it.
…east-dev#518) * Require Java 11 with Enforcer We build for Java 11 now, so the build will fail with older JDKs. Have the Enforcer plugin do that for a more lucid error message. This reverts 190e605 which was squash-merged in a larger commit. Partially addresses feast-dev#517 * Build Ingestion to target Java 8, for Beam compat As well as datatypes-java since ingestion depends on it. Java 11 is desirable for the other components, but for Beam it may impose limitations on what runners Feast can support, if it is even safe to run on an 11 JRE now. https://issues.apache.org/jira/browse/BEAM-2530 References feast-dev#517
* Allow tests on all branches * Allow unit tests to run on all branches
|
@woop this structure is a lot better indeed. I can make the change. Just to clarify, I will make it in |
Would be great @Joostrothweiler! Yea you are spot on. You just need to add the change somewhere here: https://github.com/gojek/feast/blob/master/core/src/main/java/feast/core/service/SpecService.java#L308 I'd say you probably want to have at least 2 unit tests as well. One to test that it can apply a project even if it doesn't exist. And one to test that there is a failure if the project exists but is archived. Thanks again :) |
…to default-project
What this PR does / why we need it:
Allow create_project to rerun safely #453
Which issue(s) this PR fixes:
Fixes #453
Does this PR introduce a user-facing change?: