wenjin272 commented on PR #686: URL: https://github.com/apache/flink-agents/pull/686#issuecomment-4474060000
Hi, @weiqingy. I noticed that the latest CI run still skipped `test_ollama_embedding_setup`. I tested this in my own repository's CI and found that the value of test_model was not the expected `all-minilm:22m`. <img width="2942" height="490" alt="image" src="https://github.com/user-attachments/assets/104ee1c7-fc92-4754-be45-01dffc250b87"; /> The root cause of this issue is environment variable leakage, and I have provided a detailed explanation below. I noticed that the title of this PR has been updated to “Make Ollama embedding test actually run in CI,” so I believe it is appropriate to address and fix this issue within the same PR. Alternatively, we can fix the issues with the Open and Ollama scripts in this PR alone, and submit a separate PR to address all environment variable leakage issues, including those related to the embedding model, chat model, and other environment variables. WDYT？ ## Root Cause: `OLLAMA_EMBEDDING_MODEL` env var leaked from e2e test modules In CI (`ut-python`), `test_model` in `test_ollama_embedding_model.py` resolves to `nomic-embed-text:latest` even though its hard-coded default is `all-minilm:22m`: ```python # python/flink_agents/integrations/embedding_models/local/tests/test_ollama_embedding_model.py:34 test_model = os.environ.get("OLLAMA_EMBEDDING_MODEL", "all-minilm:22m") ``` The value comes from two e2e test modules that mutate `os.environ` **at module import time** (not inside a fixture or test function): - `python/flink_agents/e2e_tests/e2e_tests_resource_cross_language/embedding_model_cross_language_test.py:43-44` - `python/flink_agents/e2e_tests/e2e_tests_resource_cross_language/vector_store_cross_language_test.py:43-44` ```python OLLAMA_MODEL = os.environ.get("OLLAMA_EMBEDDING_MODEL", "nomic-embed-text:latest") os.environ["OLLAMA_EMBEDDING_MODEL"] = OLLAMA_MODEL # ← runs at import time ``` ### Why this leaks into `ut-python` `tools/ut.sh -p` runs: ```bash pytest flink_agents -k "not e2e_tests" ... ``` `-k` only filters which tests are **executed/selected** — pytest still **imports every matching test file during the collection phase**, including the ones under `e2e_tests/`. Collection happens in alphabetical order, so `e2e_tests/` is imported before `integrations/`: 1. pytest collects `e2e_tests_resource_cross_language/*_test.py` → importing them executes `os.environ["OLLAMA_EMBEDDING_MODEL"] = "nomic-embed-text:latest"` as a side effect. 2. pytest then collects `integrations/embedding_models/local/tests/test_ollama_embedding_model.py` → its module-level `os.environ.get(...)` now reads the polluted value instead of falling back to `all-minilm:22m`. Net effect: the `-k "not e2e_tests"` filter prevents the e2e tests from **running**, but does not prevent their import-time side effects from leaking into the rest of the test process. ### Fix direction Move the `os.environ[...] = ...` assignments out of module scope in both e2e files — into a fixture, the test function body, or use `monkeypatch.setenv` — so importing the modules has no global side effects. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
