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

Fix links to source code in documentation pages #1444

Merged
merged 30 commits into from
Jun 6, 2024
Merged

Conversation

santisoler
Copy link
Member

@santisoler santisoler commented May 1, 2024

Summary

Update the linkcode_resolve function in Sphinx configuration file so the links to the source code of each function, class or method points to the actual code corresponding to that version, and not always to the code in main. Update some Azure configurations: checkout simpeg repo without shallow depth and fetching tags. Clean up the working directory before installing SimPEG so the version doesn't have an extra hash.

PR Checklist

  • If this is a work in progress PR, set as a Draft PR
  • Linted my code according to the style guides.
  • Added tests to verify changes to the code.
  • Added necessary documentation to any new functions/classes following the
    expect style.
  • Marked as ready for review (if this is was a draft PR), and converted
    to a Pull Request
  • Tagged @simpeg/simpeg-developers when ready for review.

Reference issue

What does this implement/fix?

This PR fixes a current issue in SimPEG docs: links to source code lead to the current code in the main branch, which is not the code that corresponds to the documentation. This inconsistency could lead to confusion among users: it will lead them to think they are running the code they are reading while they are not.

Additional information

TODO:

  • Remove hardcoded tag before merging
  • Revert 58a6d94

Use the current tag for `"github_version"` while configuring Sphinx.
This way, the links pointing to the source code of each function, class
or method points to the actual code corresponding to that version, and
not always to the code in `main`.
Copy link
Member

@jcapriot jcapriot left a comment

Choose a reason for hiding this comment

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

nice

Copy link

codecov bot commented May 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.33%. Comparing base (23d8cd3) to head (3f0f2c1).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1444   +/-   ##
=======================================
  Coverage   82.33%   82.33%           
=======================================
  Files         171      171           
  Lines       26019    26019           
=======================================
  Hits        21423    21423           
  Misses       4596     4596           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@santisoler
Copy link
Member Author

I'm going to hard code the github_version to the latest release and check the built docs (Azure artifact) to see if the change works as expected. If it does I'll revert that commit so we can merge this PR.

@jcapriot
Copy link
Member

jcapriot commented May 1, 2024

OH... I think i know what you're running into. Look at this line from the build: https://dev.azure.com/simpeg/simpeg/_build/results?buildId=4022&view=logs&j=a3da1655-dbd3-5466-5850-e4f85983e337&t=b6fcda75-f2ce-5ec9-7c70-5cf16be9995c&l=45

The sources that CI pulls don't include tags, so it won't get an accurate version number when it's installed.
The tagged commits due get this information.

You should be able to configure the checkout step to pull tags https://learn.microsoft.com/en-us/azure/devops/pipelines/yaml-schema/steps-checkout?view=azure-pipelines

@jcapriot jcapriot self-requested a review May 2, 2024 15:48
@santisoler
Copy link
Member Author

Nice catch! Thanks for noticing it! Fetching tags is also super relevant for #1428.

Sync tags in testing jobs, so the documentation builds as part of the
test suits is done against the correct tag. This also fix the issue of
the built artifact not being correct.
This commit should be reverted.
@santisoler
Copy link
Member Author

@santisoler
Copy link
Member Author

It worked 🎉

@jcapriot
Copy link
Member

jcapriot commented May 3, 2024

Great. Another thing is that in the testing scripts it does directly modify a tracked file (It appends a python version to the environment_test.yml file for if we want to test on different versions of python.) So there will always be the extra git hash on the testing version. We could probably adjust this step to copy the environment_test.yml into a temporary file, then modify that temporary file with the python version instead. That way it won't modify the tracked files.

In doesn't do this currently on main when building the documentation for release and documentation builds though which is why the version is correct there.

Apply my own suggestions from the comment
jcapriot and others added 3 commits May 3, 2024 12:24
Apply my own suggestions from the comment
Restore the previous configuration for `html_context`: it configures
only the links to edit files, not to the source code of classes,
functions and methods.
@santisoler
Copy link
Member Author

Ok, I totally misundertood the documentation of pytest theme (https://pydata-sphinx-theme.readthedocs.io/en/stable/user_guide/source-buttons.html). The changes to the html_context are intended for the Edit Source Code buttons, not for the links to the source code of the classes, functions and methods in the API.

Those links are configured through the linkcode_resolve function, which I updated to use the right tag.

@santisoler
Copy link
Member Author

Now the source links are working properly!

bitmap

That link won't work because the doc pages with simpeg as the package don't exist yet. But in the next release they will.

I'm removing the hardcoded tag, and after that I think this should be ready!

@santisoler santisoler changed the title Use tag in links to source code in Sphinx pages Fix links to source code in documentation pages May 9, 2024
@santisoler
Copy link
Member Author

@jcapriot, I've been seeing that the ubuntu-latest_3.8_tests/base tests/flow ... workflow gets stuck in this PR. I've run it a few times already (re-running failed jobs, updating the branch) and it always gets stuck by the end of running the tests. I've only seen it in this PR.

Might it be related to the the fact that I disabled the shallow depth while checking out the repo?

@jcapriot
Copy link
Member

It's possible it's some race condition happening with the multiprocessing tests. There are some broken pipe messages in those tests when they pass it looks like.

@santisoler
Copy link
Member Author

Right! Nice catch. I'll wait for #1464 to fix it then.

@jcapriot
Copy link
Member

I hope that fixes it, (it at least fixes the broken pipe messages).

@jcapriot
Copy link
Member

Well 💩, looks like it’s still racing away.

Revert this commit before merging.
@santisoler
Copy link
Member Author

I've just disabled pytest's capture of stdout and stderr so we can get more idea of what's going on... let's wait to see if there is some information in those messages.

@jcapriot
Copy link
Member

Not sure why but on this branch, the mappings for the dask meta aren't being sent to the correct workers: https://dev.azure.com/simpeg/simpeg/_build/results?buildId=4144&view=logs&j=894b861d-556c-5901-7578-c933b75ce341&t=19b313e8-909c-53b5-c60c-1097c881cd09&l=4252

Whereas on main they are: https://dev.azure.com/simpeg/simpeg/_build/results?buildId=4141&view=logs&j=894b861d-556c-5901-7578-c933b75ce341&t=ed1e0f4a-533f-5064-832c-7c51df369f6b&l=548

I can reproduce this locally on my machine, give me a minute to do some debugging.

jcapriot added a commit to jcapriot/simpeg that referenced this pull request May 20, 2024
@jcapriot jcapriot mentioned this pull request May 20, 2024
6 tasks
@jcapriot
Copy link
Member

Took me a bit, but #1469 should fix the race condition.

@santisoler
Copy link
Member Author

Merging this now that CI passes! Thanks @jcapriot!

@santisoler santisoler merged commit f6b8035 into main Jun 6, 2024
18 checks passed
@santisoler santisoler deleted the docs-source-tag branch June 6, 2024 18:23
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

2 participants