Skip to content

Conversation

@mikeprosserni
Copy link
Collaborator

@mikeprosserni mikeprosserni commented May 22, 2025

What does this Pull Request accomplish?

The PythonPanelService has two clients, the measurement script and the streamlit script. This PR is providing a new PanelAccessor class for the streamlit script to use to get/set values for controls. Unlike Panel, the PanelAccessor cannot open or close panels, it can only get and set values.

Note, I changed the available python versions of this project to exclude 3.9.7, because streamlit (which is now a dependency) doesn't work with 3.9.7.

Why should this Pull Request be merged?

Implements AB#3121341 - Create client library to connect Streamlit panel to Python Panel Service

What testing has been done?

Added new autotests

@github-actions
Copy link
Contributor

github-actions bot commented May 22, 2025

Test Results

 10 files  ± 0   10 suites  ±0   9s ⏱️ ±0s
 27 tests + 3   27 ✅ + 3  0 💤 ±0  0 ❌ ±0 
240 runs  +30  240 ✅ +30  0 💤 ±0  0 ❌ ±0 

Results for commit 816d98d. ± Comparison against base commit 529bcda.

♻️ This comment has been updated with latest results.

@mikeprosserni mikeprosserni changed the title [DRAFT] Add a panel accessor class for streamlit scripts to call Add a panel accessor class for streamlit scripts to call May 23, 2025
@mikeprosserni mikeprosserni marked this pull request as ready for review May 23, 2025 20:36
@mikeprosserni mikeprosserni requested a review from csjall as a code owner May 23, 2025 20:36
@mikeprosserni mikeprosserni requested review from bkeryan and Copilot May 23, 2025 20:36
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds a generic panel accessor and streamlines Streamlit panel APIs while updating dependencies

  • Introduce PanelAccessor and specialized StreamlitPanelAccessor to consolidate get/set logic
  • Refactor StreamlitPanel to inherit from PanelAccessor and use a shared constant
  • Bump Python requirement to 3.9.8 and add Streamlit dependency; update tests for the new accessor

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/unit/test_streamlit_panel.py Updated imports and added tests for StreamlitPanelAccessor
src/nipanel/_streamlit_panel_accessor.py New StreamlitPanelAccessor class wrapping PanelAccessor
src/nipanel/_streamlit_panel.py Refactored to inherit PanelAccessor and use STREAMLIT_PYTHON_PANEL_SERVICE
src/nipanel/_streamlit_constants.py New constant for the Streamlit panel service interface
src/nipanel/_panel_accessor.py New base class consolidating get/set value functionality
src/nipanel/_panel.py Refactored Panel to subclass PanelAccessor, removed duplication
src/nipanel/init.py Exported StreamlitPanelAccessor
pyproject.toml Bumped Python to ^3.9.8 and added Streamlit >=1.24 dependency
Comments suppressed due to low confidence (3)

src/nipanel/_panel.py:7

  • GrpcChannelPool is imported but unused in this file. Consider removing the import to keep the code clean.
from ni_measurement_plugin_sdk_service.grpc.channelpool import GrpcChannelPool

src/nipanel/_panel_accessor.py:22

  • The constructor docstring only documents panel_id and grpc_channel. Please add entries for discovery_client and grpc_channel_pool to fully describe all parameters.
"""Create an accessor for a Streamlit panel.

src/nipanel/_panel.py:12

  • The class references ABC but does not import it. Add from abc import ABC at the top of the file to avoid a NameError.
class Panel(PanelAccessor, ABC):

Copy link
Collaborator

@bkeryan bkeryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with suggestions

@mikeprosserni mikeprosserni merged commit ee64290 into main May 27, 2025
14 checks passed
@mikeprosserni mikeprosserni deleted the users/mprosser/task-3121341-panel-accessor branch May 27, 2025 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants