Skip to content

Conversation

@mikeprosserni
Copy link
Collaborator

@mikeprosserni mikeprosserni commented Apr 23, 2025

What does this Pull Request accomplish?

This PR implements Panel.connect() and Panel.disconnect(), allowing the Panel to connect to a PythonPanelServer. The port to connect to is determined by the specific subclass of Panel, and how it overrides _resolve_service_address().

The StreamlitPanel uses the DiscoveryClient to find the port for the "ni.pythonpanel.v1.PythonPanelService".

I also added a PortPanel for testing. This allows test code to connect to a server at a specified port. This is used in conjunction with a new FakePythonPanelServicer that trivially implements the PythonPanelServer interface.

Why should this Pull Request be merged?

Implements AB#3095680 - Implement connect() and disconnect()

What testing has been done?

I added tests for the new FakePythonPanelServicer, just to ensure it accepts requests and returns responses.

Also, the preexisting tests for Panel now use the new PortPanel (with a pytest fixture) to connect to the FakePythonPanelServicer.

@mikeprosserni mikeprosserni changed the title Fix AB#3095680 - Implement connect() and disconnect() Fixes AB#3095680 - Implement connect() and disconnect() Apr 23, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Apr 23, 2025

Test Results

10 files  ± 0  10 suites  ±0   5s ⏱️ ±0s
 8 tests + 5   8 ✅ + 5  0 💤 ±0  0 ❌ ±0 
80 runs  +50  80 ✅ +50  0 💤 ±0  0 ❌ ±0 

Results for commit 5f873b7. ± Comparison against base commit 13e7f3c.

This pull request removes 3 and adds 8 tests. Note that renamed tests count towards both.
tests.unit.test_panel ‑ test___connected_panel___set_value___gets_same_value
tests.unit.test_panel ‑ test___streamlit_panel___has_panel_id_and_panel_uri
tests.unit.test_panel ‑ test___with_panel___set_value___gets_same_value
tests.unit.test_python_panel_service_stub ‑ test___connect___gets_response
tests.unit.test_python_panel_service_stub ‑ test___disconnect___gets_response
tests.unit.test_python_panel_service_stub ‑ test___get_value___gets_response
tests.unit.test_python_panel_service_stub ‑ test___set_value___gets_response
tests.unit.test_streamlit_panel ‑ test___connected_panel___set_value___gets_same_value
tests.unit.test_streamlit_panel ‑ test___first_connect_fails___connect___gets_value
tests.unit.test_streamlit_panel ‑ test___panel___has_panel_id_and_panel_uri
tests.unit.test_streamlit_panel ‑ test___with_panel___set_value___gets_same_value

♻️ This comment has been updated with latest results.

@mikeprosserni mikeprosserni changed the title Fixes AB#3095680 - Implement connect() and disconnect() Implement connect() and disconnect() Apr 24, 2025
@mikeprosserni mikeprosserni marked this pull request as ready for review April 24, 2025 22:02
@mikeprosserni mikeprosserni requested review from bkeryan and csjall April 24, 2025 22:03
@csjall
Copy link
Collaborator

csjall commented Apr 24, 2025

Take a look at pin map client code. A couple of notable things:

  • Optionally passes in discovery client, channel pool, or channel. Allows caller to specify their own. Create these if not provided.
  • Lazily creates stub before calling rpc method.
  • Uses lock to synchronize lazy creation. Ensure that we are not creating multiple stubs at the same time.
  • Create requests using method parameters. Return value/tuples from response.
  • Uses logger to log debug traces

@bkeryan
Copy link
Collaborator

bkeryan commented Apr 25, 2025

  • Optionally passes in discovery client, channel pool, or channel. Allows caller to specify their own. Create these if not provided.

Note that channel is really for unit testing.

@mikeprosserni mikeprosserni requested review from bkeryan and csjall April 25, 2025 21:23
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 ab1f2cf into main Apr 26, 2025
13 checks passed
@mikeprosserni mikeprosserni deleted the users/mprosser/task-3095680-connect-disconnect branch April 26, 2025 02:40
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.

3 participants