Skip to content

Make the link to different object types explicit for the docs #258

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 9 commits into from
Dec 13, 2024

Conversation

ksimpson-work
Copy link
Contributor

@ksimpson-work ksimpson-work commented Nov 28, 2024

Make the object links in the docs point somewhere

  • quickly made this change in passing as I was working on adding documentation for core.experimental.system (which as a singleton instance, should certainly have a link to the System class in my opinion)

built the docs locally and was able to click my way around the objects as expected

@ksimpson-work ksimpson-work self-assigned this Nov 28, 2024
Copy link
Contributor

copy-pr-bot bot commented Nov 28, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@ksimpson-work ksimpson-work added documentation Improvements or additions to documentation P2 Low priority - Nice to have cuda.core Everything related to the cuda.core module labels Nov 28, 2024
@ksimpson-work ksimpson-work marked this pull request as ready for review November 28, 2024 23:38
@ksimpson-work
Copy link
Contributor Author

/ok to test

Copy link
Member

@leofang leofang left a comment

Choose a reason for hiding this comment

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

Thanks for catching all the missing hyperlinks, Keenan. I have one question: Is it possible for us to keep it short? Maybe we can change all the ::currentmodule entries? (not sure)

The long names (I see your already added ~ so after rendering they are still short) have two issues

  1. it's harder to write
  2. by the time we move all objects to out of the experimental module, we'd have to check all docstrings again

@ksimpson-work
Copy link
Contributor Author

that was a perfect solution

@leofang leofang added this to the cuda.core beta 2 milestone Dec 4, 2024
@ksimpson-work
Copy link
Contributor Author

/ok to test

@ksimpson-work
Copy link
Contributor Author

/ok to test

@ksimpson-work
Copy link
Contributor Author

/ok to test

.. module:: cuda.core.experimental
.. currentmodule:: cuda.core.experimental
Copy link
Member

Choose a reason for hiding this comment

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

I thought this does not need to be changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are correct, the issue was actually a missing pip install. fixed

vzhurba01
vzhurba01 previously approved these changes Dec 13, 2024
Copy link
Collaborator

@vzhurba01 vzhurba01 left a comment

Choose a reason for hiding this comment

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

LGTM but I see Leo has a final question pending

@ksimpson-work
Copy link
Contributor Author

links for ContextOptions are not working because ContextOptions is TODO and not imported. Context is not import in init and is also not linking. Do we want this behaviour, or do we want Context to link?

@leofang
Copy link
Member

leofang commented Dec 13, 2024

No need to sweat for Context (and MemoryResource too, I guess) for now as they aren't really fully public.

@leofang leofang merged commit d4418b3 into NVIDIA:main Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda.core Everything related to the cuda.core module documentation Improvements or additions to documentation P2 Low priority - Nice to have
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants