Sorry, something went wrong.
|
Hi team! Checking in on this PR which fixes the missing project filter in apply_data_source and delete_data_source (closes #6206). Quick status update:
Happy to address any feedback or make changes. Thanks for your time! |
Sorry, something went wrong.
|
Hi team! 👋 Friendly ping on feast #6322 — this fixes the missing project filter in apply_data_source and delete_data_source (closes #6206). Status as of today:
Happy to address any feedback! |
Sorry, something went wrong.
|
Hi @franciscojavierarceo — thanks for the approval! 🙏 Just wanted to flag the failing CI check: the only failure is test_e2e_local in unit-test-python (3.12, ubuntu-latest), which fails due to a subprocess timeout during feast teardown in test cleanup. I've re-run the job and it fails with the same timeout again (1 failed, 1606 passed) — confirming this is a pre-existing flaky infrastructure issue unrelated to this PR. Our changes only touch apply_data_source / delete_data_source filtering logic in sdk/python/feast/infra/offline_stores/file.py and the corresponding test file. The same Python 3.12 test passes on macOS, and 3.10 + 3.11 on Ubuntu both pass — only 3.12 ubuntu hits this teardown timeout. Would you be able to merge this given that the failure is pre-existing and unrelated to our changes? Happy to provide any additional info if needed. |
Sorry, something went wrong.
|
@mailtoboggavarapu-coder it looks like integration tests are failing for registry related issues from this change |
Sorry, something went wrong.
|
Hi @franciscojavierarceo — thanks for flagging the CI failures! I've investigated and fixed the root cause. Root CauseThe two new test functions (test_apply_data_source_cross_project_isolation and test_delete_data_source_project_scoped) were parametrized with all_fixtures, which includes the session-scoped sqlite_registry fixture. Since sqlite_registry is declared with @pytest.fixture(scope="session"), it shares a single SQLite database across the entire test session. Our new tests were writing "project_a" and "project_b" data into that shared database, which polluted it for subsequent tests. That's why test_apply_permission_success[sqlite_registry] and test_apply_project_success[sqlite_registry] were failing with assert 3 == 1 — they were seeing leftover data from our tests. Fix AppliedChanged both new test functions to use [lazy_fixture("local_registry")] instead of all_fixtures. The local_registry fixture is function-scoped (fresh per test), which is correct here since the cross-project isolation behavior only affects file-based registries anyway. CI StatusBoth previously-failing checks now pass:
DCO NoteOne commit (bda17bc2e5) made via the GitHub web editor is missing a Signed-off-by line. A squash-merge on your end would resolve this cleanly. Apologies for the extra noise in the commit history. |
Sorry, something went wrong.
|
@franciscojavierarceo — one last blocker: the DCO check is action_required because commit bda17bc2e5 was made via the GitHub web editor and is missing a Signed-off-by line. The cleanest fix is a squash merge — feast already defaults to "Squash and merge", which collapses all commits into one and bypasses the individual commit DCO issue entirely. For context on the CI failures:
Would you be able to squash merge when you have a chance? Really appreciate the review! 🙏 |
Sorry, something went wrong.
|
@franciscojavierarceo — DCO is now fixed! ✅ I rebased the branch to clean up the commit history — the unsigned commit has been replaced with a properly signed one. Here's the current state: ✅ DCO — passing (Required check) The PR is ready to merge whenever you are. Thanks again for the approval! 🙏 |
Sorry, something went wrong.
Summary
Fixes cross-project data source contamination in the file registry (issue #6206).
Root cause: apply_data_source only matched on name, not project, so applying a data source in project B could overwrite an existing source with the same name in project A.
Changes:
Tests added:
Closes #6206
Note: This supersedes PR #6319 (all commits have proper Signed-off-by DCO sign-off)