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 repo owner to doc source links #334

Merged
merged 22 commits into from
Jan 3, 2023
Merged

Add repo owner to doc source links #334

merged 22 commits into from
Jan 3, 2023

Conversation

nateraw
Copy link
Contributor

@nateraw nateraw commented Dec 5, 2022

While adding some reference docs to timm's Docs, I noticed the "Source" links were incorrect in the API documentation - they always pointed to github.com/huggingface/timm. We want them to point to github.com/rwightman/pytorch-image-models.

This PR aims to make that happen.

As of the time of creating this PR, there is an ambiguity between package and package_name, which still makes links generate incorrectly with package_name=timm, as the github repo name should still be pytorch-image-models. Will sort out a fix for that...

@nateraw
Copy link
Contributor Author

nateraw commented Dec 13, 2022

@mishig25 do you think I should go with:

  1. the route I'm doing here, where I'm parametrizing the repo name/repo owner (probably the right way to do it but I'll have to rewrite some tests and I'm afraid something in another repo will break)
  2. Just adding explicit override when package name is pytorch-image-models to update github links/package name (in which case I close this PR and make a much smaller, simpler one to hack around this)

@nateraw
Copy link
Contributor Author

nateraw commented Dec 13, 2022

@sgugger mind having a look at this and weighing in on the above?

Copy link
Contributor

@sgugger sgugger 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 working on this. I think it's better to parametrize everything, just in case we end up re-using the doc-builder for other repos outside of huggingface, like bigscience (and since you have done most of the work already ;-) )

.github/workflows/build_pr_documentation.yml Outdated Show resolved Hide resolved
src/doc_builder/autodoc.py Outdated Show resolved Hide resolved
src/doc_builder/external.py Outdated Show resolved Hide resolved
src/doc_builder/external.py Outdated Show resolved Hide resolved
src/doc_builder/external.py Outdated Show resolved Hide resolved
@nateraw nateraw marked this pull request as ready for review December 15, 2022 02:44
@nateraw nateraw requested a review from sgugger December 15, 2022 02:44
@nateraw
Copy link
Contributor Author

nateraw commented Dec 15, 2022

@sgugger, I think we're good to go here 🤞 I made updates to reflect your review comments (thanks so much!!)

I'll need to remove hard coded references to this PR's branch in the GitHub actions before we merge, though... needed to add that so I could work on huggingface/pytorch-image-models#1575.

@mishig25
Copy link
Contributor

mishig25 commented Dec 15, 2022

@nateraw, if I go to this preview page & click on one of the source links, for example:
https://github.com/rwightman/pytorch-image-models/blob/vr_1575/src/timm/optim/optim_factory.py#L176

it should not have, string src & just be:
https://github.com/rwightman/pytorch-image-models/blob/vr_1575/timm/optim/optim_factory.py#L176

if it does not have src & if you replace manually vr_1575 with main, the link works

Copy link
Contributor

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

LGTM, once @mishig25 's bug is fixed of course :-)

@nateraw
Copy link
Contributor Author

nateraw commented Dec 15, 2022

@mishig25 nice catch! I added the version_tag_suffix build arg to the workflows so I can pass it through from timm. Links look good now in that preview link you shared 😄 .

Copy link
Contributor

@mishig25 mishig25 left a comment

Choose a reason for hiding this comment

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

lgtm !

@nateraw nateraw merged commit 1e9e960 into main Jan 3, 2023
@nateraw nateraw deleted the repo-owner-bugfix branch January 3, 2023 15:12
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