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

feature: Add try_relative_path option (for Griffes) #145

Closed
pedro-psb opened this issue Mar 27, 2024 · 8 comments
Closed

feature: Add try_relative_path option (for Griffes) #145

pedro-psb opened this issue Mar 27, 2024 · 8 comments
Assignees

Comments

@pedro-psb
Copy link

Is your feature request related to a problem? Please describe.

I'm working on a project that aggregates docs from multiple repos by copying their content to a tempdir, and I have to move some namespaces around to properly lay them out logically. I'll illustrate the problem below, but TLDR, because Griffe looks for the package in the relative path by default, that interfere with my definitions and causes it to find the wrong definitions (depending on my current directory).

Now, clarifying the situation:

One of the repos, prj_cli, have a module called prj_main.cli.common and a reference to it as ::: prj_main.cli.common. That was a choice of prj_cli team, because its a "shared" plugin in the project and when installed together that makes sense. That worked well while the docs were not aggregated.

But now, they are all built together, and there is indeed a prj_main repository which has its own stuff. When making the aggregated tmpdir, I'm providing a way to merge over these $TMP/prj_cli/prj_main/cli/common to the $TMP/prj_main/ directory, so the reference prj_main.cli.common makes sense. And finally, I'm adding the prj_main tmp source location to mkdocstrings paths.

That trick is working, unless my CWD is in the true prj_main repository (for example, working on the docs in there), because then Griffe find prj_main in the relative path (and not in /tmp/.../my_prj), which doesn't contain my_prj.cli.common.

Describe the solution you'd like

To have a try_relative_path option in mkdocstring python handler to be passed to griffe.

Describe alternatives you've considered

  • Coordinate with prj_cli team to do it differently.
  • Suggest a change in Griffe so the relative_path is tried as a last resource.

Additional context

I'll happily open a PR.

@pawamoy
Copy link
Member

pawamoy commented Mar 28, 2024

Hello, thanks for the feature request!

Could you actually provide a minimum reproducible example of your issue? Because looking at the code, try_relative_path is only used to find the top module name. Finding the actual package is done here:

https://github.com/mkdocstrings/griffe/blob/01da648b10aca57d316e74e4b0eef2ca31e92f32/src/griffe/finder.py#L190

So if the search paths you provided are correct, Griffe should find your package there and not in the current working directory 🤔 At least I think that's what it would be doing. If you can provide a small repro I'll use it to confirm 🙂

@pedro-psb
Copy link
Author

mkdocstrings-reproduction.tar.gz

# setup reproduction
tar -xzvf mkdocstrings-reproduction.tar.gz --directory /tmp
cd /tmp/myprj
python -m venv venv
. venv/bin/activate
pip install mkdocs mkdocstrings[python]

# check "Reference > main" in both cases to see the difference

# A) current dirname == myprj
mkdocs serve -f /tmp/myprj/mkdocs.yml
# B) current dirname != myprj
cd /tmp/tmp
mkdocs serve -f /tmp/myprj/mkdocs.yml

@pawamoy
Copy link
Member

pawamoy commented Mar 30, 2024

Thanks a lot, I was able to replicate the issue. There's actually a side-effect in the finder's _top_module_name method: it inserts a path in front of the search paths. Ideally we'd remove this side-effect, but if I can't find something elegant we'll just go with your suggestion and add the try_relative_path option to mkdocstrings-python 🙂

@pawamoy
Copy link
Member

pawamoy commented Apr 2, 2024

Actually I think we can simply default to try_relative_path=False in the Python handler.

pawamoy added a commit that referenced this issue Apr 2, 2024
@pawamoy pawamoy closed this as completed Apr 2, 2024
@pedro-psb
Copy link
Author

Thanks!

@VArt3m
Copy link

VArt3m commented Apr 3, 2024

Uh-oh. This (starting with version 1.9.1) broke docs builder for us in python 3.8 with error messages like:

INFO    -  Cleaning site directory
INFO    -  Building documentation to directory: /home/username/projects/package_dir/public
...
DEBUG   -  Reading: API/index.md
DEBUG   -  Running 1 `page_markdown` events
DEBUG   -  mkdocstrings: Matched '::: package_name'
DEBUG   -  mkdocstrings: Using handler 'python'
DEBUG   -  mkdocstrings: Collecting data
DEBUG   -  mkdocstrings: Loaded inventory from 'https://docs.python.org/3.10/objects.inv': 9146 items
DEBUG   -  griffe: Could not find package_name
DEBUG   -  griffe: Trying inspection on package_name
...
ERROR   -  mkdocstrings: With sys.path = ['/home/username/projects/package_dir/package_name', '/home/username/projects/py38_tests/.venv/bin',
           '/opt/ros/humble/lib/python3.10/site-packages', '/opt/ros/humble/local/lib/python3.10/dist-packages', '/usr/lib/python38.zip', '/usr/lib/python3.8',
           '/usr/lib/python3.8/lib-dynload', '/home/username/projects/py38_tests/.venv/lib/python3.8/site-packages'], accessing 'package_name' raises ModuleNotFoundError: No
           module named 'package_name'
ERROR   -  Error reading page 'API/index.md':
ERROR   -  Could not collect 'package_name'

Using pip install . to install the local package fixed the issue, however it's a breaking change in a minor version (which many don't have pinned - but lesson learned), and should be probably mentioned in migration guide or somewhere else appropriate.
Tagging just in case for visibility @pawamoy
Hopefully it doesn't come off as negative or hostile, and thank you for your hard work on this great tool :)

@pawamoy
Copy link
Member

pawamoy commented Apr 3, 2024

Hi @VArt3m, no worries, and sorry for the breakage. I did think this could impact some users, but only in edge cases (misconfiguration and/or users actually changing directories before/while building to implicitly change Python's sys.path, for which we offer more robust ways).

Happy to investigate your specific issue if you can share a link to a public repo 🙂
But usually, setting the right search paths is the way to go 🙂

@VArt3m
Copy link

VArt3m commented Apr 4, 2024

@pawamoy Thanks a lot for a timely and very helpful answer :)
In the end, it was a misconfiguration indeed. Unfortunately no opensource repo, but basically:
The repo was organized as "mkdocs.yml in root, 'package_name' in root", but configuration looked like this:

handlers:
      python:
        paths: ['package_name']

By replacing with default as follows, everything works properly now.

handlers:
      python:
        paths: [.] 

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

No branches or pull requests

3 participants