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 — feat: Force absolute paths in file based registries by boliri · Pull Request #4774 · feast-dev/feast

Conversation

@boliri

@boliri boliri commented Nov 20, 2024

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

While I was working on #4772, I discovered an issue related to the Feast CLI and how FileRegistryStore instances are populated when running a CLI command using the -f argument. Assume a really simple setup made with the feast init command, i.e. the Feature Store is backed by file stores both in the registry and in the online store.

When the registry is set to file in the feature_store.yaml, RegistryConfig's path attribute is filled with the same exact value provided in the YAML, and at some point, the Registry is instantiated. This is not the same for the FileRegistryStore that is instantiated later, however - in that class, paths from the RegistryConfig are processed to turn them into absolute paths in the event they are relative:

class FileRegistryStore(RegistryStore):
    def __init__(self, registry_config: RegistryConfig, repo_path: Path):
        registry_path = Path(registry_config.path)
        if registry_path.is_absolute():
            self._filepath = registry_path
        else:
            self._filepath = repo_path.joinpath(registry_path)

    [...]

The problem here is that the tool used to build the CLI injects the current working directory in its context's CHDIR key, and that CHDIR is passed down the successive calls as the repo_path until the FileRegistryStore is finally built, which results in a malformed absolute path. Consequently, things like the UI cannot load (the landing page just claims there was an issue while loading the project list due to a malformed feature_store.yaml, but that's actually misleading).

Besides these issues, there's another one related to the env var FEATURE_STORE_YAML_BASE64. When the YAML is base-64 encoded and passed to the CLI with a file-based registry configured using a relative path, the final path built in FileRegistryStore takes as prefix a subfolder of /tmp, hence assuming that the file registry is stored in that subfolder (which is impossible, as it is created on the fly just to store the decoded env var as an actual file somewhere). Once again, the FileRegistryStore ends up in a weird state.

To mitigate these issues, I propose enforcing the usage of absolute paths while configuring the feature_store.yaml, be it a file or an env var. The onus is on the user then, and I am aware this can be a breaking change for people currently using file-based registries on their Feature Store setups, but the odds of using registries like these in production environments are very low IMHO.

Aside from that constraint, I also updated the logic behind the feast init command to replace the default values of the feature_store.yaml template with absolute paths built according to the dir from which the command was issued.

Which issue(s) this PR fixes:

Misc

Sorry, something went wrong.

Uh oh!

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

All reactions
boliri added 2 commits November 20, 2024 13:59
@boliri
feat: Force users to specify absolute paths when a File Registry is c…
00ece9c
…onfigured in feature_store.yaml
@boliri
feat: Generate absolute paths for file-based registry and online stor…
2425d69
…e when issuing 'feast init' commands
@boliri
boliri requested a review from a team as a code owner November 20, 2024 17:51
@boliri boliri changed the title Feat/force absolute paths in file based registries feat: force absolute paths in file based registries Nov 20, 2024
@boliri boliri changed the title feat: force absolute paths in file based registries feat: Force absolute paths in file based registries Nov 20, 2024
@boliri

boliri commented Nov 20, 2024

Copy link
Copy Markdown
Contributor Author

Just noticed some unit tests broke after these changes, will take a look at them when I have some time.

All reactions

Sorry, something went wrong.

Uh oh!

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

@boliri
boliri marked this pull request as draft November 20, 2024 18:02
@dmartinol

dmartinol commented Nov 21, 2024

Copy link
Copy Markdown
Contributor

Hi @boliri, I think this change goes against a recent update to use relative file paths in the generated template #4624.
This has been designed to improve code portability, which absolute paths seem to be against.

Wrt using encoded YAML in the env var, I understand the severity of the issue that you mention and we can think of enforcing the use of absolute paths there.
In any case, we expect to have a limited number of setups where the env var is requested to the user. In fact, with the advent of the new Feast Go Operator #4561, the variable will be entirely managed by the Kubernetes controller which will create the correct absolute path for you.
Helm chart users will be instead requested to encode a YAML file with the correct absolute path(s).

All reactions

Sorry, something went wrong.

Uh oh!

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

@boliri

boliri commented Nov 21, 2024

Copy link
Copy Markdown
Contributor Author

Hi @dmartinol! Thanks for the quick reply.

Didn't think about portability to be honest, but it definitely makes sense.

About the env var holding encoded YAMLs, if you don't foresee a high number of setups using it then I think we can safely drop this PR.

Let me know if you want me to do that, or feel free to close it. Thanks again!

All reactions

Sorry, something went wrong.

Uh oh!

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

@stale

stale Bot commented Apr 26, 2025

Copy link
Copy Markdown

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

All reactions

Sorry, something went wrong.

Uh oh!

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

@stale stale Bot added the wontfix This will not be worked on label Apr 26, 2025
@stale stale Bot closed this May 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

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

Assignees

No one assigned

Labels

wontfix This will not be worked on

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.

2 participants

@boliri @dmartinol
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 01402520 — 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.