-
Notifications
You must be signed in to change notification settings - Fork 26
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
Check that devices from the new API have wires set #664
Conversation
…talyst into lightning_new_api
[sc-60563] |
Co-authored-by: Ali Asadi <10773383+maliasadi@users.noreply.github.com>
…talyst into lightning_new_api
…into device_wires
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.
Thanks 👍
@pytest.mark.skipif( | ||
not pathlib.Path(get_lib_path("runtime", "RUNTIME_LIB_DIR") + "/libdummy_device.so").is_file(), | ||
reason="lib_dummydevice.so was not found.", | ||
) |
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.
It looks like this test could run even without the dummy device being present, since the check happens right on initialization (before config
and backend_info
are used).
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.
We could also think about always building the dummy device with the runtime, that way we always have at least one device available for tests like this. What do you think?
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 let me do that in another PR, good idea 👍
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.
Oh I remember why it is like that, it is because we don't build it with wheels. So I think it works well because it was added for wheels testing. Do you want us to build it in the wheels and remove the skips?
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.
So I think we could totally have the dummy device be always present (in dev builds and wheels), but also for this particular test case I don't think you need to skip because the error we are testing for is raised before Catalyst realizes the device is not there (e.g. if config
and backend_info
were just None
).
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.
We have config = device_get_toml_config(device) backend_info = extract_backend_info(device, config)
in the test and cannot pass without the shared library.
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 but my point is those values are not used and could just set them to None
.
Co-authored-by: David Ittah <dime10@users.noreply.github.com>
Context:
The new device API support devices without wires being set, we do not.
Description of the Change:
Add an additional checks that the wires are set on the provided device.