fix(script-runner): use plugin venv for Python suite analysis - #3767
fix(script-runner): use plugin venv for Python suite analysis#3767mcosgriff wants to merge 7 commits into
Conversation
Forward the selected virtual environment for temporary scripts and expose plugin site-packages when analyzing saved and temporary Python suites.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3767 +/- ##
==========================================
+ Coverage 79.38% 79.40% +0.01%
==========================================
Files 897 898 +1
Lines 67627 67652 +25
Branches 2662 2663 +1
==========================================
+ Hits 53688 53721 +33
+ Misses 13266 13263 -3
+ Partials 673 668 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Share plugin venv resolution and environment setup across script execution, suite analysis, and microservice launch paths.
Verify command timestamps fall within the command execution window instead of relying on a fixed timing tolerance.
| @@ -159,6 +160,15 @@ def self.process_suite(name, contents, new_process: true, username: nil, scope:) | |||
| # Preserve PYTHONPATH to ensure Python can find both UV venv and user packages | |||
| process.environment['PYTHONPATH'] = ENV['PYTHONPATH'] || '.' | |||
There was a problem hiding this comment.
You're setting all these values here but then clobbering them in the configure_for_script call.
There was a problem hiding this comment.
Wait the configure_for_script doesn't always override ... only if the venv resolves. Targets without a a venv need this.
There was a problem hiding this comment.
Right, only clobbers when the venv resolves. The one real overlap was PYTHONPATH - configure_environment read ENV instead of the value seeded here. Fixed in the other thread.
| environment['PYTHONUSERBASE'] = venv_dir | ||
|
|
||
| site_packages = Dir.glob("#{venv_dir}/lib/python*/site-packages").first | ||
| existing_pythonpath = ENV.fetch('PYTHONPATH', '') |
There was a problem hiding this comment.
I think you need to prefer a seeded PYTHONPATH being dropped into this method:
existing_pythonpath = environment['PYTHONPATH'] || ENV.fetch('PYTHONPATH', '')
Script.process_suite is already setting PYTHONPATH to the ENV var or '.' (not blank).
There was a problem hiding this comment.
Done. Now environment['PYTHONPATH'] || ENV.fetch('PYTHONPATH', '').
| unless python_venv_dir | ||
| system_venv = File.dirname(ENV.fetch('OPENC3_PYTHON_BIN', '/openc3/python/.venv/bin/python')).chomp('/bin') | ||
| process.environment['VIRTUAL_ENV'] = system_venv | ||
| process.environment['PYTHONUSERBASE'] = ENV.fetch('PYTHONUSERBASE', nil) |
There was a problem hiding this comment.
Instead of nil I think the default should be /gems/python_packages to match script.rb, microservice_operator.rb and microservice_model.rb
There was a problem hiding this comment.
Honestly this is probably worth a constant somewhere since it's in 4 different files
There was a problem hiding this comment.
Done. Default is now PythonVenv::DEFAULT_PYTHONUSERBASE, used in script.rb, running_script.rb, microservice_operator.rb and microservice_model.rb.
| system_venv = File.dirname(ENV.fetch('OPENC3_PYTHON_BIN', '/openc3/python/.venv/bin/python')).chomp('/bin') | ||
| process.environment['VIRTUAL_ENV'] = system_venv | ||
| process.environment['PYTHONUSERBASE'] = ENV.fetch('PYTHONUSERBASE', nil) | ||
| process.environment['PYTHONPATH'] = ENV.fetch('PYTHONPATH', nil) |
There was a problem hiding this comment.
Worth a comment to note that this is explicitly nil so it does not override with a bad path like .
There was a problem hiding this comment.
Added a comment noting the nil is deliberate so it can't default to . and let a local file shadow a real module.
There was a problem hiding this comment.
This isn't filtering by scope. Every other method takes the scope as a parameter. Change the Dir.glob to filter by scope. Users in different scopes shouldn't see python venvs in other scopes.
There was a problem hiding this comment.
Done. Takes scope now and globs <scope>__*/. Also added the same check in resolve_script_venv - a temp script could pass pythonVenv: "OTHER__demo" directly and File.basename only stripped traversal, not cross-scope.
|
Claude created the writeup. The TLDR is that we're prepending the plugins venv before the openc3 package. Thus if a plugin includes openc3 as a dependency they'll get an old version. We don't do this in our DEMO but others might cuz it sort of makes sense to have openc3 as a dependency. configure_environment builds: Plugin site-packages goes first. In the container existing_pythonpath starts with /openc3/python/.venv/lib/pythonX/site-packages (openc3-ruby/Dockerfile:168), so the plugin venv sits ahead of openc3's own. The prepend has to exist: uvinstall creates plugin venvs with plain uv venv — no --system-site-packages — and the process still runs /openc3/python/.venv/bin/python. Splicing PYTHONPATH is the only way plugin packages reach the script. The risk: any plugin venv that contains openc3 shadows the container's copy. A plugin pinned to an older openc3 silently downgrades the library talking to your current API — no error, wrong behavior wherever the API changed. What I found just now that softens it: the demo plugin already guards against this by convention. openc3-cosmos-demo/pyproject.toml puts openc3 in [dependency-groups] dev with the comment "Only needed for development/testing; provided by COSMOS at runtime", and uvinstall runs uv sync --frozen --no-dev --no-install-project — so the dev group is excluded. The reference plugin is safe, and the flags back the convention up. What's still open: nothing enforces it. A third-party plugin that lists openc3 under [project] dependencies instead of the dev group gets it installed, and it shadows, with no warning at install or run time. So the question is whether that's an acceptable documented convention or wants a guardrail:
Same block, same root cause, separate decisions: VIRTUAL_ENV and PATH are pointed at a venv whose interpreter never runs and which has no openc3 in it, so anything downstream honoring either (uv run, a bare python subprocess) gets an interpreter that can't import openc3. |
I feel like we should protect against openc3 being in the dependency list -- maybe a step in uvinstall which will fail the install and tell the user why? |
- Confine plugin venvs to the caller's scope: plugin_python_venvs now globs <scope>__*/ and resolve_script_venv rejects a temp script venv from another scope, which File.basename did not catch - Prefer a PYTHONPATH already seeded on environment in configure_environment so Script.process_suite's value is not lost - Default PYTHONUSERBASE to PythonVenv::DEFAULT_PYTHONUSERBASE instead of nil, replacing the literal in the four files that repeated it - Use PythonVenv.plugin_venv_path in MicroserviceModel#runtime_python_env rather than rebuilding the sanitized path by hand Refs #3732 Co-Authored-By: Claude noreply@anthropic.com
…ite-analysis-does-not-use-venv
Apply the same tr() PluginModel.plugin_venv_name builds venv directory fix(python-venv): whitelist temp script venv name characters Apply the same tr() PluginModel.plugin_venv_name builds venv directory names with, so glob metacharacters cannot survive into the Dir.glob in configure_environment. File.basename and the scope prefix check both pass a name like "DEFAULT__*"; only File.directory? stopped it, which made the guarantee depend on filesystem state rather than the value. Refs #3732 Co-Authored-By: Claude noreply@anthropic.com
|
4 participants
Forward the selected virtual environment for temporary scripts and expose plugin site-packages when analyzing saved and temporary Python suites.
What changed
Test suite wasn't passing the virtual environment
Why it changed
Closes #3732
Testing strategy
Installed cowsay package in the test plugin venv, imported that library
import cowsayand addedprint(cowsay.get_output_string('cow', 'Hello World'))to the setup method to thePowergroupReview notes
Similar logic for running scripts in script runner