-
Notifications
You must be signed in to change notification settings - Fork 0
Use the measurement script's environment for the streamlit script #98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
Pull Request Overview
This PR ensures the Streamlit panel uses the same Python interpreter as the measurement script by adding a python_path field throughout the service stack, cleans up dependencies and example scripts, and updates documentation.
- Propagate
python_pathfromPanelinitializer throughPanelClientto the gRPC service - Extend protobuf definitions and regenerate serialized descriptors
- Refine dependencies, example scripts, and contributing instructions
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_panel_client.py | Updated tests to pass python_path to start_panel |
| src/nipanel/_panel_client.py | Extended start_panel signature to include python_path |
| src/nipanel/_panel.py | Resolve and pass sys.executable path into start_panel |
| src/ni/pythonpanel/v1/python_panel_service_pb2.pyi | Added python_path field to the StartPanelRequest stub interface |
| src/ni/pythonpanel/v1/python_panel_service_pb2.py | Updated serialized descriptor ranges to accommodate python_path |
| pyproject.toml | Removed streamlit-echarts from main deps; adjusted numpy constraints |
| protos/ni/pythonpanel/v1/python_panel_service.proto | Added python_path field to StartPanelRequest message |
| examples/simple_graph/simple_graph_panel.py | Moved get_panel_accessor after set_page_config |
| examples/nidaqmx/nidaqmx_continuous_analog_input_panel.py | Adjusted sample_rate default, added st_echarts key |
| examples/nidaqmx/nidaqmx_continuous_analog_input.py | Added panel URL printout; formatted task.read call |
| examples/hello/hello_panel.py | Moved get_panel_accessor after set_page_config |
| examples/all_types/all_types_panel.py | Moved get_panel_accessor after set_page_config |
| CONTRIBUTING.md | Updated example setup and run instructions |
Comments suppressed due to low confidence (3)
src/nipanel/_panel_client.py:62
- The new required
python_pathparameter is a breaking change for existing callers; consider making it optional (e.g., with a default value or keyword-only) to maintain backward compatibility.
def start_panel(self, panel_id: str, panel_script_path: str, python_path: str) -> str:
tests/unit/test_panel_client.py:16
- Tests invoke
start_panelwithpython_pathbut do not assert that the stub request actually received the expectedpython_path; add a test to verify the request payload includes the correct interpreter path.
client.start_panel("panel1", "uri1", "python.exe")
pyproject.toml:15
- The examples still use
st_echartsbut its dependency was removed from main deps; consider addingstreamlit-echartsto anexamplesordevdependency group so example scripts run out of the box.
streamlit = ">=1.24"
What does this Pull Request accomplish?
We need to send the measurement script's python environment to the PythonPanelServer, so that it can reuse it for the Streamlit script as well. See the ASW PR for the full explanation.
On the python side here, we need to:
python_panel_service.prototo includepython_pathin theStartPanelRequeststr(Path(sys.executable).resolve())to get thepython_pathinPanel.__init__python_paththroughPanelClient.start_panel()Additional Changes:
streamlit-echartsas a required dependency (it's only needed for examples), and fix an issue I was getting withnumpywhen runningpoetry lockpanel = nipanel.get_panel_accessor()belowst.set_page_config(), because in older versions of streamlit,set_page_configneeds to be the first thing in the script (andget_panel_accessoradds the refresh component)Why should this Pull Request be merged?
Addresses AB#3171565
What testing has been done?
See ASW PR for description and animations of testing