Skip to content

DPCTLSyclInterface should avoid functions that print to std::cout #542

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

Merged
merged 2 commits into from
Oct 6, 2021

Conversation

oleksandr-pavlyk
Copy link
Contributor

The stream std::cout that the library writes to in a function like
DPCTLDeviceMgr_PrintDeviceInfo is not the same as Python's sys.stdout.

It makes it difficult to verify that printed output is what should be
expected.

Used OutputGrabber class to try to capture std::cout outputs of DPCTLSyclInterface.
The class is implemented in new test/_redirector.py file.

The changes in this commit allow the test to pass, while ensuring
that the captured output is non-empty, but pytest must be envoked with
--capture=no option.

It works as follows:

(idp) [14:36:03 ansatnuc04 dpctl]$ ipython
Python 3.7.10 (default, Jun  4 2021, 06:52:02)
Type 'copyright', 'credits' or 'license' for more information
IPython 7.25.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import dpctl

In [2]: q = dpctl.SyclQueue()

In [3]: q.print_device_info()
    Name            Intel(R) UHD Graphics [0x9bca]
    Driver version  1.1.20389
    Vendor          Intel(R) Corporation
    Profile         FULL_PROFILE
    Filter string   level_zero:gpu:0

In [4]: import tests._redirector

In [5]: out = tests._redirector.OutputGrabber()

In [6]: with out: q.print_device_info()

In [7]: out.capturedtext
Out[7]: '    Name            Intel(R) UHD Graphics [0x9bca]\n    Driver version  1.1.20389\n    Vendor          Intel(R) Corporation\n    Profile         FULL_PROFILE\n    Filter string   level_zero:gpu:0\n'

It is all good, but In[3] for needed for Out[7] to be non-empty.

(idp) [14:38:36 ansatnuc04 dpctl]$ ipython
Python 3.7.10 (default, Jun  4 2021, 06:52:02)
Type 'copyright', 'credits' or 'license' for more information
IPython 7.25.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import dpctl

In [2]: q = dpctl.SyclQueue()

In [3]: import tests._redirector

In [4]: out = tests._redirector.OutputGrabber()

In [5]: with out: q.print_device_info()

In [6]: out.capturedtext
Out[6]: ''

This is because libDPCTLSyclInterface does not initialize std::cout, and In[5] writes into a different std::cout that output grabber captured.

@oleksandr-pavlyk
Copy link
Contributor Author

I just pushed a change to use function added in #620 . Once that is merged, this becomes ready for review.

@oleksandr-pavlyk oleksandr-pavlyk marked this pull request as ready for review October 6, 2021 00:51
@oleksandr-pavlyk oleksandr-pavlyk force-pushed the cleanup/test_sycl_queue branch 3 times, most recently from 63c2f7b to 8fc1535 Compare October 6, 2021 02:16
@coveralls
Copy link
Collaborator

coveralls commented Oct 6, 2021

Coverage Status

Coverage increased (+0.01%) to 74.424% when pulling b6e7e23 on cleanup/test_sycl_queue into 782340f on master.

dpctl.SyclDevice.print_device_info gets CString from DPCTL-CAPI and
prints it using Python's stream.
@oleksandr-pavlyk oleksandr-pavlyk force-pushed the cleanup/test_sycl_queue branch from e91235d to f4f3b51 Compare October 6, 2021 17:45
@oleksandr-pavlyk
Copy link
Contributor Author

OS sycl bundle from 2021-10-06 does not contain CL/sycl/version.hpp instance of template CL/sycl/version.hpp.in, so please disregard that failure.

@oleksandr-pavlyk oleksandr-pavlyk force-pushed the cleanup/test_sycl_queue branch from f4f3b51 to b6e7e23 Compare October 6, 2021 18:34
Copy link
Contributor

@diptorupd diptorupd left a comment

Choose a reason for hiding this comment

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

Unless you want to wait for TeamCity to finish, feel free to merge.

@oleksandr-pavlyk
Copy link
Contributor Author

I ended up removing use of OutputGrabber in favor of pytest's own stream capture fixture. Since Python API only outputs to Python's stream, pytest works well and there is no need for the OutputGrabber .

@oleksandr-pavlyk oleksandr-pavlyk merged commit 711f868 into master Oct 6, 2021
@oleksandr-pavlyk oleksandr-pavlyk deleted the cleanup/test_sycl_queue branch October 6, 2021 18:50
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