GeezerVison

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

GeezerVison is showing github.com — test: Stabilize CLI runner subprocess path by franciscojavierarceo · Pull Request #6560 · feast-dev/feast

Conversation

franciscojavierarceo commented Jun 26, 2026

Copy link

Copy Markdown

Member

What changed

Normalizes the environment used by CliRunner subprocesses in unit tests.
Adds the parent pytest process sys.path entries to PYTHONPATH before invoking feast.cli.cli in a child Python process.
Applies the same environment to both subprocess.run(...) and subprocess.Popen(...) helper paths.

Why

The unit-test workflow flaked on unit-test-python (3.12, ubuntu-latest) while setting up test_push_source_does_not_exist. The parent pytest process had imported Feast successfully, but the child feast apply subprocess failed before the test body with:

ModuleNotFoundError: No module named 'google.protobuf.timestamp_pb2'

That points at the subprocess import environment rather than the test assertion. Passing through the parent process import path makes the CLI subprocess use the same repo/site-packages view as pytest.

Validation

uv run ruff check sdk/python/tests/utils/cli_repo_creator.py
uv run python -m pytest sdk/python/tests/unit/test_feature_server.py::test_push_source_does_not_exist -q
subprocess smoke: from google.protobuf.timestamp_pb2 import Timestamp using CliRunner._subprocess_env()

Note: local setup required make install-python-dependencies-ci PYTHON_VERSION=3.12 because this machine does not have a python shim for the Makefile's default version detection.

Sorry, something went wrong.

Uh oh!

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

All reactions


          test: Stabilize CLI runner subprocess path

79d89d6

Signed-off-by: Francisco Javier Arceo <[email protected]>

franciscojavierarceo marked this pull request as ready for review

June 26, 2026 15:27

franciscojavierarceo requested a review from a team as a code owner

June 26, 2026 15:27

franciscojavierarceo merged commit 141c08c into master

Jun 26, 2026

27 checks passed

Uh oh!

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

chatgpt-codex-connector Bot reviewed

Jun 26, 2026

View reviewed changes

chatgpt-codex-connector Bot left a comment

Copy link

Copy Markdown

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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 79d89d66e5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

sdk/python/tests/utils/cli_repo_creator.py

		parent_paths = [path for path in sys.path if path]
		existing_pythonpath = env.get("PYTHONPATH")
		if existing_pythonpath:
		parent_paths.append(existing_pythonpath)

chatgpt-codex-connector Bot Jun 26, 2026

Copy link

Copy Markdown

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.

Preserve child PYTHONPATH precedence

In workflows where PYTHONPATH intentionally contains an empty/current-directory entry for the CLI cwd (for example, .github/workflows/unit_tests.yml exports sdk/python:${PYTHONPATH}:$SITE_PACKAGES), appending the raw existing value after the parent sys.path moves parent site-packages and repo-root paths ahead of the temporary feature repo. Since cli_check_repo() only appends the feature repo path later, imports for repo-local providers or feature definition modules can resolve to an installed/root module with the same name, so CliRunner no longer exercises the isolated repo. Keep the original PYTHONPATH entries first, or add only parent-only paths after the child cwd entry.

Useful? React with 👍 / 👎.

Sorry, something went wrong.

Uh oh!

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

All reactions

nikolauspschuetz mentioned this pull request

Jul 9, 2026

test: Retry CliRunner apply subprocess on timeout to reduce flakiness #6592

Merged

ntkathole pushed a commit to nikolauspschuetz/feast that referenced this pull request

Jul 14, 2026


          test: Retry CliRunner apply subprocess on timeout to reduce flakiness

e74df8e

CliRunner.run() (used for 'feast apply' in unit tests) spawns a fresh child
interpreter that must import Feast (pandas/pyarrow/...) from cold. On loaded
CI runners — observed on unit-test-python (3.11, macos-14) — this can exceed
the 120s timeout, failing the test with:

    AssertionError: Command timed out after 120s: ['apply']

Unlike the materializing paths guarded by run_with_output(), commands routed
through run() do not materialize and so cannot trigger the Dask atexit hang the
timeout was added for. A timeout here is therefore almost always transient cold
-start slowness, not a genuine deadlock, so retrying once (with warm OS/page
caches) clears it while a real repeated stall still fails after the attempts
are exhausted.

Builds on the CliRunner subprocess stabilization in feast-dev#6560.

Signed-off-by: Nikolaus Schuetz <[email protected]>

ntkathole pushed a commit to nikolauspschuetz/feast that referenced this pull request

Jul 30, 2026


          test: Retry CliRunner apply subprocess on timeout to reduce flakiness

9aca1af

CliRunner.run() (used for 'feast apply' in unit tests) spawns a fresh child
interpreter that must import Feast (pandas/pyarrow/...) from cold. On loaded
CI runners — observed on unit-test-python (3.11, macos-14) — this can exceed
the 120s timeout, failing the test with:

    AssertionError: Command timed out after 120s: ['apply']

Unlike the materializing paths guarded by run_with_output(), commands routed
through run() do not materialize and so cannot trigger the Dask atexit hang the
timeout was added for. A timeout here is therefore almost always transient cold
-start slowness, not a genuine deadlock, so retrying once (with warm OS/page
caches) clears it while a real repeated stall still fails after the attempts
are exhausted.

Builds on the CliRunner subprocess stabilization in feast-dev#6560.

Signed-off-by: Nikolaus Schuetz <[email protected]>

ntkathole pushed a commit to nikolauspschuetz/feast that referenced this pull request

Jul 30, 2026


          test: Retry CliRunner apply subprocess on timeout to reduce flakiness

CliRunner.run() (used for 'feast apply' in unit tests) spawns a fresh child
interpreter that must import Feast (pandas/pyarrow/...) from cold. On loaded
CI runners — observed on unit-test-python (3.11, macos-14) — this can exceed
the 120s timeout, failing the test with:

    AssertionError: Command timed out after 120s: ['apply']

Unlike the materializing paths guarded by run_with_output(), commands routed
through run() do not materialize and so cannot trigger the Dask atexit hang the
timeout was added for. A timeout here is therefore almost always transient cold
-start slowness, not a genuine deadlock, so retrying once (with warm OS/page
caches) clears it while a real repeated stall still fails after the attempts
are exhausted.

Builds on the CliRunner subprocess stabilization in feast-dev#6560.

Signed-off-by: Nikolaus Schuetz <[email protected]>

ntkathole pushed a commit that referenced this pull request

Jul 30, 2026


          test: Retry CliRunner apply subprocess on timeout to reduce flakiness

6019bd6

CliRunner.run() (used for 'feast apply' in unit tests) spawns a fresh child
interpreter that must import Feast (pandas/pyarrow/...) from cold. On loaded
CI runners — observed on unit-test-python (3.11, macos-14) — this can exceed
the 120s timeout, failing the test with:

    AssertionError: Command timed out after 120s: ['apply']

Unlike the materializing paths guarded by run_with_output(), commands routed
through run() do not materialize and so cannot trigger the Dask atexit hang the
timeout was added for. A timeout here is therefore almost always transient cold
-start slowness, not a genuine deadlock, so retrying once (with warm OS/page
caches) clears it while a real repeated stall still fails after the attempts
are exhausted.

Builds on the CliRunner subprocess stabilization in #6560.

Signed-off-by: Nikolaus Schuetz <[email protected]>

Reviewers

chatgpt-codex-connector[bot]

chatgpt-codex-connector[bot] left review comments

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

1 participant

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.