GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix: Added Apply/Get/List/Delete methods to project_metadata to BaseRegistry by EXPEbdodla · Pull Request #4441 · feast-dev/feast

Conversation

@EXPEbdodla

@EXPEbdodla EXPEbdodla commented Aug 24, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

  1. Adds Apply/Get/List/Delete methods to project_metadata to BaseRegistry
  2. Made project as Optional in list_project_metadata method so we can use it retrieve all projects when project is None. Can be deleted in future if necessary.
  3. Renamed _maybe_init_project_metadata to apply_project_metadata. Registry Server will have use cases to create projects in future.
  4. I'll implement the methods to remote registry in future PRs

Which issue(s) this PR fixes:

Misc

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tokoko reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Bhargav Dodla added 2 commits August 23, 2024 16:41
Bhargav Dodla
feat: Added project metadata specific methods to base registry
785d7b2
Signed-off-by: Bhargav Dodla <[email protected]>
Bhargav Dodla
fix: Fixed lint issues
9d8fdac
Signed-off-by: Bhargav Dodla <[email protected]>
@EXPEbdodla EXPEbdodla changed the title Add project endpoints fix: Added Apply/Get/List/Delete methods to project_metadata to BaseRegistry Aug 24, 2024
@EXPEbdodla EXPEbdodla mentioned this pull request Aug 24, 2024
fix: Optimize SQL Registry proto() method #4425
Closed
Bhargav Dodla
fix: Fixed lint issues in remote and registry files
cc673e3
Signed-off-by: Bhargav Dodla <[email protected]>
@tokoko tokoko added the ok-to-test label Aug 24, 2024
tokoko
tokoko reviewed Aug 24, 2024
View reviewed changes
Comment thread sdk/python/feast/infra/registry/base_registry.py
raise NotImplementedError

@abstractmethod
def apply_project_metadata(

@tokoko tokoko Aug 24, 2024

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Wouldn't it make more sense for this to also accept ProjectMetadata object as an input similar to other registry resources? I know the object doesn't hold much information right now, but if we plan for it to be "updatable", an update should happen through this method, right?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@EXPEbdodla EXPEbdodla Aug 24, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I was not envisioning this as a updatable. At this point its create only. Fields we have for this are UUID and LastUpdateTimestamp. UUID should be constant. LastUpdateTimestamp should be update when any changes happen to any objects associated to the project.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@tokoko

tokoko commented Aug 24, 2024

Copy link
Copy Markdown
Collaborator

This probably means we will have to treat ProjectMetadata as another one of FeastObject types and should be added in here. Similarly for rbac to work, we need it here as well, but that can be deferred until handling in remote registry is implemented. @dmartinol

Another point... if we treat apply_project_metadata as basically project creation and delete_project_metadata as deletion, along with all the contents (implementations seem to indicate this), wouldn't better name for this new feast object be Project rather than ProjectMetadata?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Bhargav Dodla added 3 commits August 24, 2024 11:08
Bhargav Dodla
fix: Fixed test failures
6bddbf8
Signed-off-by: Bhargav Dodla <[email protected]>
Bhargav Dodla
fix: Revert experimental optional field on ListProjectMetadataRequest…
54beea5
… and using default string

Signed-off-by: Bhargav Dodla <[email protected]>
Bhargav Dodla
fix: Default to empty when project is None
23b279f
Signed-off-by: Bhargav Dodla <[email protected]>
@EXPEbdodla

EXPEbdodla commented Aug 24, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Another point... if we treat apply_project_metadata as basically project creation and delete_project_metadata as deletion, along with all the contents (implementations seem to indicate this), wouldn't better name for this new feast object be Project rather than ProjectMetadata?

@tokoko You are right. We can introduce the Project object with spec and metadata. We can use the Project and fade out ProjectMetadata. Should I proceed with changing the PR accordingly?

And I always saw Project as a parent entity for all the FEAST Objects.

Having a Separate Project object will also help us to load or preserve the feature_store.yaml associated to the project as we progress further.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@EXPEbdodla

EXPEbdodla commented Aug 24, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

This probably means we will have to treat ProjectMetadata as another one of FeastObject types and should be added in here.

We won't treat this as an object that users define in Feature Definitions. We can populate project name from feature_store.yaml. At this point, I'm not aware of any use cases for this.
If we let users define Project in definitions, we need to ensure name in feature_store.yaml match with Project object in definitions and ensure we can only have one Project is defined per Repo. This will enable users to specify/ update tags, owner, description, any other metadata we would like to add.
Defining Project object may be a case for RBAC. I have no idea about this at this point. Looking forward for the suggestions on this.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tokoko

tokoko commented Aug 25, 2024

Copy link
Copy Markdown
Collaborator

Defining Project object may be a case for RBAC. I have no idea about this at this point. Looking forward for the suggestions on this.

yup, that's what I think as well. A lot of rbac revolves around tagging, so it makes sense to make it possible to tag projects and consequently we will have to let users update those tags. Maybe we can make it so that users are able to define Project objects in the repo, but revert to auto-generating one for them if they don't do it.

@franciscojavierarceo @HaoXuAI Any thoughts/objections about introducing Project feast object and phasing out ProjectMetadata in favor of it? I think it makes a lot of sense especially considering rbac.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dmartinol

dmartinol commented Aug 26, 2024

Copy link
Copy Markdown
Contributor

This probably means we will have to treat ProjectMetadata as another one of FeastObject types and should be added in here. Similarly for rbac to work, we need it here as well, but that can be deferred until handling in remote registry is implemented. @dmartinol

Agreee, while developing the registry_server we'll also need to assert the required permissions.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@haoxu0

haoxu0 commented Aug 27, 2024

Copy link
Copy Markdown
Collaborator

No objection. Can we have multiple projects per repo where different project use different online/offline store setup?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@EXPEbdodla

EXPEbdodla commented Aug 27, 2024

Copy link
Copy Markdown
Contributor Author

No objection. Can we have multiple projects per repo where different project use different online/offline store setup?

@HaoXuAI We are discussing about the multiple Projects in the issue: #4199. Project object can help us enable multiple projects per repo.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tokoko

tokoko commented Sep 17, 2024

Copy link
Copy Markdown
Collaborator

@EXPEbdodla this is no longer relevant, right?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@franciscojavierarceo

franciscojavierarceo commented Sep 27, 2024

Copy link
Copy Markdown
Member

Is this PR still relevant? Thanks for doing this @EXPEbdodla and sorry for the complications here.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@EXPEbdodla

EXPEbdodla commented Sep 27, 2024

Copy link
Copy Markdown
Contributor Author

Not relevant. I'll close it.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@EXPEbdodla EXPEbdodla closed this Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tokoko tokoko tokoko left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@EXPEbdodla @tokoko @dmartinol @haoxu0 @franciscojavierarceo
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.