Skip to content

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

Merged
oleksandr-pavlyk merged 2 commits intomasterfrom
cleanup/test_sycl_queue
Oct 6, 2021
Merged

DPCTLSyclInterface should avoid functions that print to std::cout#542
oleksandr-pavlyk merged 2 commits intomasterfrom
cleanup/test_sycl_queue

Conversation

@oleksandr-pavlyk
Copy link
Copy Markdown
Contributor

@oleksandr-pavlyk oleksandr-pavlyk commented Aug 20, 2021

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.

@diptorupd diptorupd mentioned this pull request Oct 5, 2021
Merged
@oleksandr-pavlyk oleksandr-pavlyk force-pushed the cleanup/test_sycl_queue branch from 3500184 to 49f2378 Compare October 5, 2021 21:32
@oleksandr-pavlyk
Copy link
Copy Markdown
Contributor Author

oleksandr-pavlyk commented Oct 5, 2021

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
Copy Markdown
Collaborator

coveralls commented Oct 6, 2021 •
edited
Loading

Coverage Status

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

diptorupd
diptorupd reviewed Oct 6, 2021
View reviewed changes
Comment thread dpctl/_sycl_device.pyx
@oleksandr-pavlyk
use GetDeviceInfoStr
821e6a1 
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
Copy Markdown
Contributor Author

oleksandr-pavlyk commented Oct 6, 2021

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
diptorupd
diptorupd approved these changes Oct 6, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

oleksandr-pavlyk commented Oct 6, 2021

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

Reviewers

1 more reviewer

@diptorupd diptorupd diptorupd approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@oleksandr-pavlyk @coveralls @diptorupd