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

Overhaul of dpctl documentation #1619

Merged
merged 58 commits into from
Apr 17, 2024
Merged

Overhaul of dpctl documentation #1619

merged 58 commits into from
Apr 17, 2024

Conversation

oleksandr-pavlyk
Copy link
Collaborator

@oleksandr-pavlyk oleksandr-pavlyk commented Mar 28, 2024

This PR is a rewrite of dpctl's documentation. Here is the link to rendered docs based on sources in this PR.

  1. It introduces Beginner's guide, User Guide, Reference Guide and contributor's guide
  2. It does away with generate_rst.py and AutoAutosummary class, but uses Sphinx's autosummary extension to generate individual pages for classes, and array API functions.
  3. It changes theme from read-the-docs to Furo
  4. It modifies docstrings to fix sphinx issues
  5. It modifies array API function signatures by using positional-only, keyword-only delimiters as done in the standard.
    This required test changes and code changes.
  • Have you provided a meaningful PR description?
  • Have you added a test, reproducer or referred to an issue with a reproducer?
  • Have you tested your changes locally for CPU and GPU devices?
  • Have you made sure that new changes do not introduce compiler warnings?
  • Have you checked performance impact of proposed changes?
  • If this PR is a work in progress, are you opening the PR as a draft?

@diptorupd
Copy link
Contributor

@oleksandr-pavlyk the PR will need to be split up. I suggest all docstring changes to be merged separately.

Copy link

github-actions bot commented Mar 28, 2024

Deleted rendered PR docs from intelpython.github.com/dpctl, latest should be updated shortly. 🤞

Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_181 ran successfully.
Passed: 838
Failed: 1
Skipped: 92

1 similar comment
Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_181 ran successfully.
Passed: 838
Failed: 1
Skipped: 92

Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_181 ran successfully.
Passed: 838
Failed: 1
Skipped: 92

@coveralls
Copy link
Collaborator

coveralls commented Mar 28, 2024

Coverage Status

coverage: 88.049%. remained the same
when pulling 1abd650 on docstring-edits
into 10b831c on master.

Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_182 ran successfully.
Passed: 838
Failed: 1
Skipped: 92

@oleksandr-pavlyk oleksandr-pavlyk marked this pull request as draft March 28, 2024 17:14
Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_186 ran successfully.
Passed: 839
Failed: 0
Skipped: 92

Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_187 ran successfully.
Passed: 839
Failed: 0
Skipped: 92

Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_189 ran successfully.
Passed: 839
Failed: 0
Skipped: 92

Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_191 ran successfully.
Passed: 839
Failed: 0
Skipped: 92

@oleksandr-pavlyk oleksandr-pavlyk changed the base branch from master to docstring-edits-only April 1, 2024 13:23
@oleksandr-pavlyk
Copy link
Collaborator Author

@oleksandr-pavlyk the PR will need to be split up. I suggest all docstring changes to be merged separately.

I split the PR into gh-1625, and remaining docs-only changes in this PR opened into branch behind gh-1625.

Copy link

github-actions bot commented Apr 1, 2024

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_190 ran successfully.
Passed: 839
Failed: 0
Skipped: 92

Copy link

github-actions bot commented Apr 1, 2024

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_191 ran successfully.
Passed: 839
Failed: 0
Skipped: 92

Copy link

github-actions bot commented Apr 1, 2024

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_192 ran successfully.
Passed: 839
Failed: 0
Skipped: 92

Copy link

github-actions bot commented Apr 2, 2024

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_199 ran successfully.
Passed: 839
Failed: 0
Skipped: 92

Copy link

github-actions bot commented Apr 2, 2024

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_200 ran successfully.
Passed: 839
Failed: 0
Skipped: 92

Copy link

github-actions bot commented Apr 2, 2024

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_201 ran successfully.
Passed: 839
Failed: 0
Skipped: 92

Copy link

github-actions bot commented Apr 2, 2024

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_202 ran successfully.
Passed: 839
Failed: 0
Skipped: 92

@oleksandr-pavlyk oleksandr-pavlyk marked this pull request as ready for review April 16, 2024 15:03
Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_285 ran successfully.
Passed: 870
Failed: 8
Skipped: 92


.. note::
Two instances :class:`dpctl.SyclQueue` may target the same ``sycl::device`` and be using the same ``sycl::context``, but correspond
to different scheduling enties, and hence be in violation of the compute-follows-data requirement. One common example of this are
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit, but was this meant to say "entries"?

Comment on lines 210 to 216
If assignign USM-type "device" a score of 0, USM-type "shared" a score of 1, and USM-type "host" a score of 2,
the USM-type of the output array has the smallest score of all its inputs.

.. currentmodule:: dpctl.utils

The convenience function :py:func:`get_coerced_usm_type` is a convenience function to determine the USM-type
following this convention:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
If assignign USM-type "device" a score of 0, USM-type "shared" a score of 1, and USM-type "host" a score of 2,
the USM-type of the output array has the smallest score of all its inputs.
.. currentmodule:: dpctl.utils
The convenience function :py:func:`get_coerced_usm_type` is a convenience function to determine the USM-type
following this convention:
If assigning USM-type "device" a score of 0, USM-type "shared" a score of 1, and USM-type "host" a score of 2,
the USM-type of the output array has the smallest score of all its inputs.
.. currentmodule:: dpctl.utils
The convenience function :py:func:`get_coerced_usm_type` determines the USM-type following this convention:

# r3 has value "host"
r3 = get_coerced_usm_type(["host", "host", "host"])

Sharing data between devices and Python
Copy link
Collaborator

Choose a reason for hiding this comment

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

It may be worthwhile to add an example demonstrating what happens when migrating a Numpy array of a data type not supported by the destination device.

Can be done later though.

-------------

* History of ``"dpctl"`` :ref:`name <beginners_guide_why_dpctl>`
* Frequenty asked questions
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Frequenty asked questions
* Frequently asked questions

@@ -0,0 +1,69 @@
.. _beginners_guide_why_dpctl:

History of ``dpctl`` name
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this page should either be renamed or moved to the beginner's guides index page in the future. It stands out when looking at the list.

Comment on lines 163 to 165
:class:`dpctl.tensor.usm_ndarray` objects one associated with ``q1`` and another associated with ``q2``
could not be combined in a call to the same function that implementes
:ref:`compute-followed-data programming model <dpctl_tensor_compute_follows_data>` in :mod:`dpctl.tensor`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
:class:`dpctl.tensor.usm_ndarray` objects one associated with ``q1`` and another associated with ``q2``
could not be combined in a call to the same function that implementes
:ref:`compute-followed-data programming model <dpctl_tensor_compute_follows_data>` in :mod:`dpctl.tensor`.
:class:`dpctl.tensor.usm_ndarray` objects one associated with ``q1`` and another associated with ``q2``
could not be combined in a call to the same function that implements
:ref:`compute-follows-data programming model <dpctl_tensor_compute_follows_data>` in :mod:`dpctl.tensor`.

x1 : dpctl.tensor.usm_ndarray,
x2 : dpctl.tensor.usm_ndarray
):
exec_q = dpctil.utils.get_execution_queue((x1.sycl_queue, x2.sycl_queue))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
exec_q = dpctil.utils.get_execution_queue((x1.sycl_queue, x2.sycl_queue))
exec_q = dpctl.utils.get_execution_queue((x1.sycl_queue, x2.sycl_queue))

...

In order to ensure that compute-follows-data works seemlessly out-of-the-box, :mod:`dpctl` maintains
a cache of with context and device as keys and queues as values used by :class:`dpctl.tensor.Device` class.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
a cache of with context and device as keys and queues as values used by :class:`dpctl.tensor.Device` class.
a cache with context and device as keys and queues as values used by :class:`dpctl.tensor.Device` class.

const auto &all_root_devs = sycl::device::get_devices();
auto beg = std::begin(all_root_devs);
auto end = std::end(all_root_devs);
auto selectot_fn = [parent_root_device](const sycl::device &root_d) -> bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
auto selectot_fn = [parent_root_device](const sycl::device &root_d) -> bool {
auto selector_fn = [parent_root_device](const sycl::device &root_d) -> bool {

select_cpu_device
select_gpu_device
select_accelerator_device
select_device_with_aspects
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function could use a proper docstring later.

Copy link
Collaborator

@ndgrigorian ndgrigorian Apr 16, 2024

Choose a reason for hiding this comment

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

This was referring specifically to select_device_with_aspects, which has a description that isn't a true docstring (no real info about what types are permitted for a required_aspect, for instance).

Comment on lines +30 to +31
SyclProgram
SyclKernel
Copy link
Collaborator

Choose a reason for hiding this comment

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

Empty __init__() methods are picked up in the documentation of these, and the methods don't have pages or proper docstrings.

:py:mod:`dpctl.tensor` is written using C++ and :sycl_spec_2020:`SYCL <>`
and oneAPI extensions implemented in :dpcpp_compiler:`Intel(R) oneAPI DPC++ compiler <>`.

This module contains:
Copy link
Collaborator

@ndgrigorian ndgrigorian Apr 16, 2024

Choose a reason for hiding this comment

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

Some undocumented things:

  • all constants (pi, e, nan, inf, and newaxis)
  • asnumpy and to_numpy
  • dtype itself
  • in elementwise functions, no documentation is rendered for nin, nout, types, etc. special attributes
  • print utilities do not have rendered documentation
  • flags object on usm_ndarray is mentioned under usm_ndarray.flags but has no rendered documentation
  • order keyword in general/memory layouts (C-contiguous vs. F-contiguous) may be worth a quick note

@ndgrigorian
Copy link
Collaborator

ndgrigorian commented Apr 16, 2024

@oleksandr-pavlyk
In the future it may also be nice to change the libsyclinterface documentation rendering so that the global functions and so on are listed directly under the "on this page" section, with shortcuts. That way you don't have to scroll and click on them in the overview.

@oleksandr-pavlyk oleksandr-pavlyk changed the title WIP: overhaul of dpctl documentation Overhaul of dpctl documentation Apr 17, 2024
Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_291 ran successfully.
Passed: 871
Failed: 7
Skipped: 92

Copy link
Collaborator

@ndgrigorian ndgrigorian left a comment

Choose a reason for hiding this comment

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

Changes requested in my review have now either been committed or will be deferred to a later PR.

This LGTM, thank you @oleksandr-pavlyk! Very exciting to see these documentation improvements finally go in.

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.

Let's merge it as-is for now, the changes in the PR are already a big improvement over status quo.

Also, the PR has become too large and all further change should be done incrementally.

@oleksandr-pavlyk oleksandr-pavlyk merged commit 182aede into master Apr 17, 2024
60 checks passed
@oleksandr-pavlyk oleksandr-pavlyk deleted the docstring-edits branch April 17, 2024 19:10
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.

5 participants