Added source code link #172

ayyucedemirbas · 2022-10-04T21:31:35Z

All of the links take you to the _hf_hub.py file. I must add specific lines.

added lines

adrinjalali · 2022-10-05T07:57:57Z

Thanks for the pull request @ayyucedemirbas , please run black and isort on your modified python files to format them correctly.

Alternatively, you can enable pre-commit hooks to do that automatically when you commit code: https://github.com/skops-dev/skops/blob/main/CONTRIBUTING.rst#using-condamamba

adrinjalali

Thanks @ayyucedemirbas , this looks quite good already.

docs/conf.py

adrinjalali · 2022-10-07T10:23:33Z

docs/conf.py

+    if not info["module"]:
+        return None


this is checked bellow again (info.get("module") is safer than info["module"] since the latter can raise an exception if the key doesn't exist.

Now it works, I updated the PR. Could you please take a look at it? Thank you so much for everything!

adrinjalali · 2022-10-07T10:37:06Z

docs/conf.py

+    except Exception:
+        lineno = ""
+    return (
+        "https://github.com/skops-dev/skops/blob/main/skops/" + fn + "#L" + str(lineno)


this shouldn't refer to the main branch, it should refer to the branch for which this documentation is being built. For example, in sklearn in this page for ClassifierMixin you get this link which is not on main.

I added these lines from scikit-learn:

REVISION_CMD = "git rev-parse --short HEAD" def _get_git_revision(): try: revision = subprocess.check_output(REVISION_CMD.split()).strip() except (subprocess.CalledProcessError, OSError): print("Failed to execute git to get revision") return None return revision.decode("utf-8")

def linkcode_resolve(domain, info): ... revision = _get_git_revision() if revision is None: return ... return ( "https://github.com/skops-dev/skops/blob/{revision}/skops/" + fn + "#L" + str(lineno) )

However, the source code links became something like this (https://github.com/skops-dev/skops/blob/%7Brevision%7D/skops/hub_utils/_hf_hub.py#L328) and of course, they don't work

Could you try this patch:

diff --git a/docs/conf.py b/docs/conf.py index 3ed773f..4ed5d3d 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -8,6 +8,7 @@ import inspect import os +import subprocess from operator import attrgetter from packaging.version import parse @@ -71,6 +72,19 @@ autosummary_generate = True issues_github_path = "skops-dev/skops" +# -- linkcode configuration -------------------------------------------------- +REVISION_CMD = "git rev-parse --short HEAD" + + +def _get_git_revision(): + try: + revision = subprocess.check_output(REVISION_CMD.split()).strip() + except (subprocess.CalledProcessError, OSError): + print("Failed to execute git to get revision") + return None + return revision.decode("utf-8") + + def linkcode_resolve(domain, info): if domain != "py": return None @@ -93,7 +107,7 @@ def linkcode_resolve(domain, info): try: fn = inspect.getsourcefile(inspect.unwrap(obj)) except TypeError: - try: + try: fn = inspect.getsourcefile(inspect.unwrap(obj.fget)) except (AttributeError, TypeError): fn = None @@ -105,9 +119,16 @@ def linkcode_resolve(domain, info): lineno = inspect.getsourcelines(obj)[1] except Exception: lineno = "" - return ( - "https://github.com/skops-dev/skops/blob/main/skops/" + fn + "#L" + str(lineno) + url_fmt = ( + "https://github.com/scikit-learn/" + "scikit-learn/blob/{revision}/" + "{package}/{path}#L{lineno}" ) + revision = _get_git_revision() + return url_fmt.format(revision=revision, package=package, path=fn, lineno=lineno) + + +# -- linkcode configuration -------------------------------------------------- # -- Options for HTML output -------------------------------------------------

Returns something like this: https://github.com/scikit-learn/scikit-learn/blob/a6db88f/skops/hub_utils/_hf_hub.py#L594
and also doesn't work

There are two problems why this doesn't work. First of all it links to sklearn but we want to link to skops (I guess Adrin forgot to adjust the copied code):

url_fmt = ( "https://github.com/skops-dev/" "skops/blob/{revision}/" "{package}/{path}#L{lineno}" )

But even after that, the link wouldn't lead anywhere. However, this is probably because the git revision's hash does not lead anywhere (yet). If we replace it with the hash of an actual commit (current HEAD on main branch), it will work, e.g.:

https://github.com/skops-dev/skops/blob/69621cd/skops/hub_utils/_hf_hub.py#L594

where 69621cd is the current git rev-parse --short HEAD. So when you test the link locally, it might not work because the commit doesn't exist on GitHub. But that's no problem, once the code is merged, the links will be correct.

Ahahahahhahahhahah I feel like an idiot. Thank you so much!

BenjaminBossan · 2022-10-13T12:12:29Z

docs/conf.py

@@ -65,6 +70,46 @@
 # (note that `group` is the GitHub user or organization)
 issues_github_path = "skops-dev/skops"

+
+def linkcode_resolve(domain, info):
+    if domain != "py":


Adrin already mentioned this, but this line and the one below

if domain not in ("py", "pyx"):

do redundant work. Only the latter is necessary.

BenjaminBossan · 2022-10-13T12:22:38Z

docs/conf.py

+        return None
+    if not info["module"]:
+        return None
+    filename = info["module"].replace(".", "/")


filename is not being used and can be deleted.

BenjaminBossan · 2022-10-13T12:23:28Z

docs/conf.py

+    try:
+        fn = inspect.getsourcefile(inspect.unwrap(obj))
+    except TypeError:
+        try: 


Suggested change

try:

try:

BenjaminBossan · 2022-10-13T12:27:47Z

docs/conf.py

+    except Exception:
+        lineno = ""
+    return (
+        "https://github.com/skops-dev/skops/blob/main/skops/" + fn + "#L" + str(lineno)


There are two problems why this doesn't work. First of all it links to sklearn but we want to link to skops (I guess Adrin forgot to adjust the copied code):

url_fmt = ( "https://github.com/skops-dev/" "skops/blob/{revision}/" "{package}/{path}#L{lineno}" )

But even after that, the link wouldn't lead anywhere. However, this is probably because the git revision's hash does not lead anywhere (yet). If we replace it with the hash of an actual commit (current HEAD on main branch), it will work, e.g.:

https://github.com/skops-dev/skops/blob/69621cd/skops/hub_utils/_hf_hub.py#L594

where 69621cd is the current git rev-parse --short HEAD. So when you test the link locally, it might not work because the commit doesn't exist on GitHub. But that's no problem, once the code is merged, the links will be correct.

BenjaminBossan

It looks good now, the documentation successfully links to the code, thanks a lot. I have only two minor comments, one about the domain check and one would be for you to add an entry to changes.rst.

BenjaminBossan · 2022-10-13T14:31:19Z

docs/conf.py

-    if domain not in ("py", "pyx"):
-        return


I would keep this check, we just don't need the if domain != "py": too, as it's redundant.

All done! I couldn't edit the changes.rst file in this pr, so I had to create a new one, which is #186

I couldn't edit the changes.rst file in this pr

That's strange. What error did you get? Maybe we can figure it out :)

There is no error. The problem is I don't know how to do that :)

Hmm, same as with the conf.py? You modify the changes.rst, then git add changes.rst, then git commit -m "blabla" and git push.

Thanks! I use the browser. Is there any way to do that on the browser? Or I'll use the CLI.

Hmm, I think it's probably possible in the browser, but I have never used it, so maybe not.

I get this error message:

! [rejected] main -> main (fetch first) error: failed to push some refs to 'https://github.com/ayyucedemirbas/skops.git' hint: Updates were rejected because the remote contains work that you do hint: not have locally. This is usually caused by another repository pushing hint: to the same ref. You may want to first integrate the remote changes hint: (e.g., 'git pull ...') before pushing again. hint: See the 'Note about fast-forwards' in 'git push --help' for details.

Perhaps, doing that through the browser would be the best.

Do a git pull, git will merge the changes made here to your local branch. That should work without further complications because there should be no merge conflicts. After that, try git push again.

Yes, I did it (via browser) :) Thanks a lot!

BenjaminBossan · 2022-10-13T15:34:18Z

I couldn't edit the changes.rst file in this pr

That's strange. What error did you get? Maybe we can figure it out :)

BenjaminBossan

Thanks a lot for your work and patience. There was a minor merge conflict that I solved on your branch, this should be ready to be merged now.

ayyucedemirbas · 2022-10-13T16:37:07Z

Thank you so much for everything!

ayyucedemirbas added 2 commits October 5, 2022 00:26

Update conf.py

2cd3a1c

all done

b3a1f4c

added lines

ayyucedemirbas marked this pull request as ready for review October 4, 2022 21:55

ayyucedemirbas added 2 commits October 5, 2022 01:06

some minor changes

b59d98c

Redundant imports are removed

fce3a1b

runned black and isort on the conf.py

5312b14

merveenoyan requested a review from adrinjalali October 7, 2022 10:12

adrinjalali reviewed Oct 7, 2022

View reviewed changes

BenjaminBossan reviewed Oct 13, 2022

View reviewed changes

All problems are solved

ff41416

BenjaminBossan requested changes Oct 13, 2022

View reviewed changes

Added domain check

7d643a2

ayyucedemirbas and others added 2 commits October 13, 2022 19:13

Update changes.rst

e9dca16

Merge branch 'main' into main

e0e254b

BenjaminBossan approved these changes Oct 13, 2022

View reviewed changes

BenjaminBossan merged commit 1020c81 into skops-dev:main Oct 14, 2022

BenjaminBossan mentioned this pull request Oct 14, 2022

Added pr:172 #186

Closed

BenjaminBossan mentioned this pull request Nov 4, 2022

Add source links to API documentation #171

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added source code link #172

Added source code link #172

ayyucedemirbas commented Oct 4, 2022

adrinjalali commented Oct 5, 2022

adrinjalali left a comment

adrinjalali Oct 7, 2022

ayyucedemirbas Oct 7, 2022

ayyucedemirbas Oct 13, 2022

adrinjalali Oct 7, 2022

ayyucedemirbas Oct 7, 2022 •

edited

adrinjalali Oct 7, 2022

ayyucedemirbas Oct 12, 2022

BenjaminBossan Oct 13, 2022

ayyucedemirbas Oct 13, 2022

BenjaminBossan Oct 13, 2022

ayyucedemirbas Oct 13, 2022

BenjaminBossan Oct 13, 2022

ayyucedemirbas Oct 13, 2022

BenjaminBossan Oct 13, 2022

ayyucedemirbas Oct 13, 2022

BenjaminBossan Oct 13, 2022

BenjaminBossan left a comment

BenjaminBossan Oct 13, 2022

ayyucedemirbas Oct 13, 2022

ayyucedemirbas Oct 13, 2022

BenjaminBossan Oct 13, 2022

ayyucedemirbas Oct 13, 2022

BenjaminBossan Oct 13, 2022

ayyucedemirbas Oct 13, 2022

ayyucedemirbas Oct 13, 2022

BenjaminBossan Oct 13, 2022

ayyucedemirbas Oct 13, 2022

BenjaminBossan commented Oct 13, 2022

BenjaminBossan left a comment

ayyucedemirbas commented Oct 13, 2022

Added source code link #172

Added source code link #172

Conversation

ayyucedemirbas commented Oct 4, 2022

adrinjalali commented Oct 5, 2022

adrinjalali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayyucedemirbas Oct 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenjaminBossan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenjaminBossan commented Oct 13, 2022

BenjaminBossan left a comment

Choose a reason for hiding this comment

ayyucedemirbas commented Oct 13, 2022

ayyucedemirbas Oct 7, 2022 •

edited