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

Add direct links to github source code to docs #7738

Merged
merged 5 commits into from
May 16, 2024

Conversation

johnzielke
Copy link
Contributor

Description

This adds direct links to the respective source code of classes, methods, etc. to the docs.
In my opinion these nicer and more useful than viewing the source code in copy in the docs.
image

I currently added it in addition to the links to the source files copied into the docs. If desired this could also be done instead of.
I also used a github logo instead of another [source] link.
I am not sure whether there is any easier way to change that into the logo than the way I have done, but this one seems to work.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

Signed-off-by: John Zielke <j.l.zielke@gmail.com>
Signed-off-by: John Zielke <j.l.zielke@gmail.com>
docs/_static/custom.css Outdated Show resolved Hide resolved
@johnzielke
Copy link
Contributor Author

johnzielke commented May 6, 2024

In response to @KumoLiu and as a general question:
If we link to the github source, do we want to even keep the existing "viewcode" extension, i.e. the copied version of the source code in the docs? I kept this mainly for backwards compatibility, IMHO it makes sense to switch exclusively to the links in github.
Or would you rather just have the [source] link go directly to github and remove "viewcode"?

I did a quick search on what other repos use this style of source code linking (using this search), and examples include:

  • scikit-learn
  • jax
  • numpy

I mainly styled the link to github as a github icon to differentiate it from the other [source] link (from viewcode). If we use the github link exclusively, it's personal preference IMO. I think that the icon uses up a little less space and makes it clear you are linking to an "external" site, but on the other hand [source] is widely used already. And changing to the icon is a bit hacky (which is also why I think almost no one has bothered doing it so far) since the linkcode extension does not easily support it.

@KumoLiu KumoLiu requested a review from zephyrie May 7, 2024 14:05
@ericspod
Copy link
Member

I think this is an improvement in general, with two things to discuss:

  1. Should we keep the existing generated source code view? If not we remove "sphinx.ext.viewcode" from the Sphinx config file so that only the Github link shows up. This is slightly less helpful if people want to generate offline docs but this is a minor point, I just think it's confusing having both links.
  2. The choice of line ranges to link to isn't always correct. For example, going to docs/build/html/networks.html#unet in the generated docs and clicking the Github link wil take one to the correct line range for UNet, but clicking on docs/build/html/networks.html#resnet produces an incorrect line range. It seems to be just below the docstring but other things I've seen have started mid-string rather than just below. Let's double check the algorithm here for computing that range.

Thanks @johnzielke for this. @zephyrie please a look to see if this makes sense for our documentation, thanks.

@johnzielke
Copy link
Contributor Author

johnzielke commented May 10, 2024

Thanks @ericspod. I agree with both points. Do you know of any people creating offline docs? If we really needed those, we can of course create a Env variable or similar to change what we generate.

Regarding the second point, if there is agreement to merge this I'll have a look at some of the mentioned repositories again and see if there are some cases that this function doesn't cover (decorators might be one of those). Another thing I have to check up on is how the mapping to source code positions might be improved by newer python versions.
And thanks for the examples, I'll check those.
While of course we should fix as many errors as possible, IMO a small error in the mapping is no deal breaker, if it's just one or two lines.

PS: I was considering creating a small pip package (sphinx extension) that does this and would prevent having to copy this into all repos needing this, since it'squite a common use case nowadays. But I'm not sure how the sentiment for an extra dev dependency for something like this would be in light of recent events with xz.

@ericspod
Copy link
Member

Thanks @ericspod. I agree with both points. Do you know of any people creating offline docs? If we really needed those, we can of course create a Env variable or similar to change what we generate.

I don't think people are but some feedback from others would be helpful before making this change.

Regarding the second point, if there is agreement to merge this I'll have a look at some of the mentioned repositories again and see if there are some cases that this function doesn't cover (decorators might be one of those). Another thing I have to check up on is how the mapping to source code positions might be improved by newer python versions. And thanks for the examples, I'll check those. While of course we should fix as many errors as possible, IMO a small error in the mapping is no deal breaker, if it's just one or two lines.

The example I gave for an incorrect line range wasn't using decorators in the definition, so I suspect there's just something mildly wrong with the algorithm that can be fixed. Perhaps someone else has it in their versions of this plugin so yeah checking around would help. If not we can still merge this since I think it's an improvement over the reformatted code.

PS: I was considering creating a small pip package (sphinx extension) that does this and would prevent having to copy this into all repos needing this, since it'squite a common use case nowadays. But I'm not sure how the sentiment for an extra dev dependency for something like this would be in light of recent events with xz.

This would be a dependency of the sphinx environment and not for MONAI so much less impactful. If it's something other people would want it could be a good project to start. New dependencies and libraries are unavoidable so one famous example of an attempted exploit shouldn't put us off. We should be cautious and have some way of verifying contributions, but that's a community-wide issue.

@johnzielke
Copy link
Contributor Author

johnzielke commented May 13, 2024

Ok great, than I'll wait for feedback from the rest.

The example I gave for an incorrect line range wasn't using decorators in the definition, so I suspect there's just something mildly wrong with the algorithm that can be fixed. Perhaps someone else has it in their versions of this plugin so yeah checking around would help. If not we can still merge this since I think it's an improvement over the reformatted code.

Thanks for noticing, in the end the error wasn't in the algorithm but in the default branch that I hard-coded.

This would be a dependency of the sphinx environment and not for MONAI so much less impactful. If it's something other people would want it could be a good project to start. New dependencies and libraries are unavoidable so one famous example of an attempted exploit shouldn't put us off. We should be cautious and have some way of verifying contributions, but that's a community-wide issue.

I will try to get a package like this available. Once it's ready, I'll open up another PR here to switch to that, but in the meantime, the simple implementation here should suffice.

I now removed the viewcode extension and removed the usage of the icon. Now the generated docs look like this, linking to the github repo.
Screenshot 2024-05-13 at 4 25 36 PM

@zephyrie
Copy link
Collaborator

I haven't seen any demand for offline documentation, so removing the viewcode extension seems reasonable, and any minor errors in mapping line ranges can be addressed.

And I think linking directly to GitHub source code is a great idea and it offers a better user experience overall!

Copy link
Member

@ericspod ericspod left a comment

Choose a reason for hiding this comment

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

I think this looks good now and the links go to the right places for me. Thanks all!

@KumoLiu
Copy link
Contributor

KumoLiu commented May 15, 2024

/build

@KumoLiu KumoLiu merged commit 1bcf97f into Project-MONAI:dev May 16, 2024
28 checks passed
@johnzielke
Copy link
Contributor Author

@KumoLiu On https://docs.monai.io/en/latest/metrics.html I am now not seeing any more links to source anymore on "latest". Is this normal or is there some error in the build? I was unable to find the CI job deploying the pages.

@KumoLiu
Copy link
Contributor

KumoLiu commented May 16, 2024

@KumoLiu On https://docs.monai.io/en/latest/metrics.html I am now not seeing any more links to source anymore on "latest". Is this normal or is there some error in the build? I was unable to find the CI job deploying the pages.

here is the job link: https://github.com/Project-MONAI/MONAI/actions/runs/9069259715/job/24918422082

@johnzielke
Copy link
Contributor Author

I think the issue is with read the docs. See #7779

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.

None yet

4 participants