Skip to content
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

service: Add generic create_session(s) API #426

Merged
merged 24 commits into from
Oct 3, 2023

Conversation

bkeryan
Copy link
Collaborator

@bkeryan bkeryan commented Sep 30, 2023

What does this Pull Request accomplish?

Add the generic create_session(s) API and some of the infrastructure needed for the driver-specific APIs.

session_management:

  • Add TSession and TSession_co type variables
  • Add session: object = None to SessionInformation to allow returning a session object along with the session info
  • Add a TypedSessionInformation[TSession_co] protocol to allow returning session objects in a type-safe manner
    • Python versions <3.11 don't support generic named tuples, so I think a protocol is the best we can do.
    • The protocol doesn't exactly match the SessionInformation class because TSession_co is narrower than object, so a cast is needed.
  • Add the generic create_session and create_sessions methods.
    • The core implementation is split out to hide the use of @contextlib.contextmanager from documentation and hover help

_drivers:

  • New subpackage for driver-related code
  • Provides a closing_session function to use to create a context manager to close the session

_drivers._grpcdevice:

  • Provides a get_insecure_grpc_device_channel function

Why should this Pull Request be merged?

Part of the new session management API.

What testing has been done?

Ran mypy and tests.

@github-actions
Copy link

github-actions bot commented Sep 30, 2023

Test Results

     12 files  ±    0       12 suites  ±0   3m 38s ⏱️ + 1m 5s
   304 tests +  22     238 ✔️ +  22    66 💤 ±0  0 ±0 
3 636 runs  +264  2 828 ✔️ +264  808 💤 ±0  0 ±0 

Results for commit f3719a6. ± Comparison against base commit 032ad4d.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@dixonjoel dixonjoel 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 a few queries and relatively minor points. Since I'm out early this week, I don't want to hold anything up so feel free to bypass me. Maybe @pbirkhol-ni should take a quick look before submitting.

Copy link
Contributor

@pbirkhol-ni pbirkhol-ni left a comment

Choose a reason for hiding this comment

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

Looks good to me. I didn't see any functional discrepancies between LabVIEW and python, although the implementation itself is radically different. :)

@bkeryan bkeryan merged commit b8346fc into main Oct 3, 2023
@bkeryan bkeryan deleted the users/bkeryan/create-sessions branch October 3, 2023 19:39
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