-
Notifications
You must be signed in to change notification settings - Fork 196
[executor-preview] Fix the executor and test it #2525
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
base: executor_preview
Are you sure you want to change the base?
Conversation
| if isinstance(mode, (Session, Batch)): | ||
| self._session = mode | ||
| self._backend = self._session._backend | ||
| self._service = self._session.service | ||
|
|
||
| elif open_session := get_cm_session(): | ||
| if open_session != mode: | ||
| if open_session._backend != mode: | ||
| raise ValueError( | ||
| "The backend passed in to the primitive is different from the session " | ||
| "backend. Please check which backend you intend to use or leave the mode " | ||
| "parameter empty to use the session backend." | ||
| ) | ||
| logger.warning( | ||
| "A backend was passed in as the mode but a session context manager " | ||
| "is open so this job will run inside this session/batch " | ||
| "instead of in job mode." | ||
| ) | ||
| self._session = open_session | ||
| self._backend = self._session._backend | ||
| self._service = self._session.service | ||
|
|
||
| elif isinstance(mode, IBMBackend): | ||
| self._backend = mode | ||
| self._service = self._backend.service | ||
|
|
||
| else: | ||
| raise ValueError( | ||
| "A backend or session/batch must be specified, or a session/batch must be open." | ||
| ) |
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.
Is this equal to what's inside get_mode_service_backend, or are there differences?
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.
There is one difference: if get_cm_session() is not None and mode is None, previously there was an error, now it will take the backend from the session.
SamFerracin
left a comment
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.
LGTM, just want to make sure that I am not missing anything in. the lines of code that you removed
| ) | ||
|
|
||
|
|
||
| class TestGetModeServiceBackend(IBMTestCase): |
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.
Should this be in test_base_primitive.py?
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.
Yes, because the function that it tests resides in base_primitive.py. I tried to move around the functions and its test in several ways, by starting new files, moving to existing utils files, etc. Everything that I did ended in circular imports that required substantial modifications to unresolve, resulting in effort and amount of modifications that were not justified.
Co-authored-by: Samuele Ferracin <[email protected]>
Part of #2479.
This PR copies code fixes and tests not-yet-approved from the open PR #2498. I suggest to first review #2498, let me fix here in parallel according to the comments of #2498, and only then review this PR.