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

Add support for allow_version_mismatch (L1-2516) #287

Merged
merged 2 commits into from
Nov 25, 2024

Conversation

fabiorossetto
Copy link
Collaborator

@fabiorossetto fabiorossetto commented Nov 21, 2024

Expose the flag so that once it's changed to False by default, it can be overridden.

Description:

Fixes issue: #

Checklist:

  • Add tests for the change to show correct behavior.
  • Add or update relevant docs, code and examples.
  • Update CHANGELOG.rst with relevant information and add the issue number.
  • Add .. versionchanged:: where necessary.

Copy link
Member

@tobiasah tobiasah left a comment

Choose a reason for hiding this comment

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

Are we sure we want to introduce this flag with default True? If I am not mistaken the idea is that when its first released the flag will be False in zhinst.core ... So wouldn't it make more sense to have the default set to False in toolkit already?

src/zhinst/toolkit/session.py Outdated Show resolved Hide resolved
Comment on lines 997 to 1013
# Attempt to pass the allow_version_mismatch flag. Fallback in case
# zhinst.core does not support it yet.
def _create_daq(
self,
server_host: str,
server_port: int,
allow_version_mismatch: bool,
):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Attempt to pass the allow_version_mismatch flag. Fallback in case
# zhinst.core does not support it yet.
def _create_daq(
self,
server_host: str,
server_port: int,
allow_version_mismatch: bool,
):
def _create_daq(
self,
server_host: str,
server_port: int,
allow_version_mismatch: bool,
):
"""Create a new session to the data server.
Attempt to pass the allow_version_mismatch flag. Fallback in case
zhinst.core does not support it yet."""

src/zhinst/toolkit/driver/modules/shfqa_sweeper.py Outdated Show resolved Hide resolved
@fabiorossetto
Copy link
Collaborator Author

Are we sure we want to introduce this flag with default True? If I am not mistaken the idea is that when its first released the flag will be False in zhinst.core ... So wouldn't it make more sense to have the default set to False in toolkit already?

My idea was to do a two stages release. First with the flag present but disabled by default, the a second commit that "flips" the condition to enabled by default.

For toolkit, maybe I can merge the "flipping" commit within this same PR

@fabiorossetto fabiorossetto force-pushed the fabior/version_mismatch branch 4 times, most recently from 0ebc513 to 9c9a47b Compare November 25, 2024 10:41
Copy link
Member

@tobiasah tobiasah left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines 296 to 304

@staticmethod
def _create_daq(server_host, server_port):
try:
return ziDAQServer(server_host, server_port, 6, allow_version_mismatch=True)
except TypeError as error:
if "allow_version_mismatch" not in error.args[0]:
raise
return ziDAQServer(server_host, server_port, 6)
Copy link
Member

Choose a reason for hiding this comment

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

issue: dead code right?

@fabiorossetto fabiorossetto force-pushed the fabior/version_mismatch branch 2 times, most recently from 2dff201 to 6f92343 Compare November 25, 2024 11:23
Fabio Rossetto added 2 commits November 25, 2024 12:26
Expose the flag so that once it's changed to False by default, it can be
overridden.
@fabiorossetto fabiorossetto force-pushed the fabior/version_mismatch branch from 6f92343 to 6c16f7d Compare November 25, 2024 11:26
@fabiorossetto fabiorossetto merged commit 46efda1 into main Nov 25, 2024
11 checks passed
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.

2 participants