jbms / sphinx-immaterial

Adaptation of the popular mkdocs-material material design theme to the sphinx documentation system
https://jbms.github.io/sphinx-immaterial/
Other
196 stars 29 forks source link

add simple pybind11 example for testing #145

Closed 2bndy5 closed 2 years ago

2bndy5 commented 2 years ago

add a simple pybind11 pkg, titled "issue_134" for testing.

This doesn't include a new pytest to build the docs for this simple pybind11 pkg (yet)

2bndy5 commented 2 years ago

I'm not sure how to install this pkg for tests without using tox. extras_require doesn't seem to like local paths (tried using file URL).

I'm actually drawing a blank as to how to make use of this test pkg. My initial assumption is to just parse the rst:

.. autoclass:: issue_134.Example
    :members:
jbms commented 2 years ago

The module could be named sphinx_immaterial_pybind11_issue_134 to make it unique. And then as part of the CI build we could just add an extra step to: pip install -e tests/issue-134

2bndy5 commented 2 years ago

I was thinking of something a little more automated like pip install -r tests/requirements.txt, that way we could use the command to install any other test projects... Say we want to start testing docs for pybind11 w/ custom function signatures or some other sphinx extension (like breathe).

Is there a reason you want an editable install?

If you'd allow a pyproject.toml for this test pkg, I could move the pybind11 dep into [build-system].requires instead of having it in the dev's env.

jbms commented 2 years ago

I think those are both good suggestions --- pyproject.toml is fine and putting it in a requirements file also seems good

2bndy5 commented 2 years ago

The CI is now going to fail because #134 is reproduced in the tests. Note this is meant to get merged to fix-134 branch (not main).

2bndy5 commented 2 years ago

This test fails locally. I'm not sure what's happening the CI (for some reason it is passing).

Some of the other tests that use immaterial_make_app() also seem to not get properly built in CI.

jbms commented 2 years ago

I'm also not seeing the failure locally. Linux with Sphinx 5.1.1, Python 3.10.5.

2bndy5 commented 2 years ago

It still fails for me locally in WSL and Windows (using python v3.10.6 and sphinx v5.1.1 in each) Just yesterday, I upgraded my python installs to v3.10.6 from v3.10.4 (on windows) & v3.10.0 (on WSL via python src - not apt pkgs). It failed for me locally before the updated python installs.

This test should be using the latest release of pybind11. My project that first showed the problem is using pybind11 v2.10.0 (the latest stable) as a submodule. I do not have pybind11 otherwise installed.

2bndy5 commented 2 years ago

I just rebooted into my Ubuntu partition, and ran the tests on this branch again. It still failed locally using python v3.10.4 & sphinx v5.1.1. The navadapt test required me to manually install pyyaml for some reason.

To run these tests I'm using v0.8.1 of this theme. Maybe something changed between that tag and this branch's HEAD.

jbms commented 2 years ago

I just rebooted into my Ubuntu partition, and ran the tests on this branch again. It still failed locally using python v3.10.4 & sphinx v5.1.1. The navadapt test required me to manually install pyyaml for some reason.

Looks like I forgot to include pyyaml in dev-requirements.txt --- it is (indirectly) pulled in by docs/requirements.txt but it should be in dev-requirements.txt (or tests/requirements.txt).

To run these tests I'm using v0.8.1 of this theme. Maybe something changed between that tag and this branch's HEAD.

I'm a bit confused --- you aren't running the tests from (a dev install of) this branch itself? This branch is based on fix-134 so it includes the fix there.

2bndy5 commented 2 years ago

Ok, the test passes when I use this branch's HEAD, and the test's generated HTML output is as expected. I had to uncomment https://github.com/jbms/sphinx-immaterial/blob/da09144601e32f92070b5fd78b6ce42d33e6473d/tests/issue_134/issue_134_test.py#L28 for the c'tor to show. I'm not particularly happy about blindly catching exceptions in the fix-134 branch, but if it works then it works. However, a similar solution could be to just ignore ValueError or TypeError:

try: 
    # ...
except (TypeError, ValueError):
    pass

I also tested this branch with the issue-134 pkg modified to use custom signatures (and still show the overloaded __init__()), and it passed gloriously. I also pushed this change for better test coverage (letting us know when pybind11 overloads and custom function signatures are suffering from future changes).


In the end, it is probably a good thing that I know the test fails in v0.8.1. Otherwise what's the point of adding it? 😆

jbms commented 2 years ago

Ok, the test passes when I use this branch's HEAD, and the test's generated HTML output is as expected. I had to uncomment

https://github.com/jbms/sphinx-immaterial/blob/da09144601e32f92070b5fd78b6ce42d33e6473d/tests/issue_134/issue_134_test.py#L28

for the c'tor to show. I'm not particularly happy about blindly catching exceptions in the fix-134 branch, but if it works then it works. However, a similar solution could be to just ignore ValueError or TypeError:

try: 
    # ...
except (TypeError, ValueError):
    pass

I think we don't really want any exceptions to pass through because any exception will prevent the documentation build from working at all. But it could make sense to log an error in some cases. In general though it may be hard to predict what the error type will be, so I'm inclined to just leave it as is, and rely on test cases it make sure it works in the cases we care about.

I also tested this branch with the issue-134 pkg modified to use custom signatures (and still show the overloaded __init__()), and it passed gloriously. I also pushed this change for better test coverage (letting us know when pybind11 overloads and custom function signatures are suffering from future changes).

In the end, it is probably a good thing that I know the test fails in v0.8.1. Otherwise what's the point of adding it? laughing

Agreed --- it is good practice to verify that the tests fail without the fix applied --- I am glad though that we resolved the confusion as to why there was a difference between the local runs and github actions runs.

However, I'm still a bit confused. In this comment: https://github.com/jbms/sphinx-immaterial/pull/142#issuecomment-1213856716 you said that the problem remained even with the fix from #142.

2bndy5 commented 2 years ago

In this comment: #142 (comment) you said that the problem remained even with the fix from #142.

That's what had me confused too. I thought I installed the built wheel artifact from that PR before testing it. I'll try it again on the original repo that showed the problem, and then try it on this test pkg. Maybe there's a difference.

2bndy5 commented 2 years ago

Back into research mode. It appears that comment is correct when testing the original pyrf24 repo against the fix in #142. There may be something else that should be added to the test pkg here -- I'm guessing different types of instance properties like def_readwrite. I still doubt it is anything to do instance methods (or overloaded methods) because applying the suggested fix in https://github.com/jbms/sphinx-immaterial/pull/142#issuecomment-1216481744 solved it for the pyrf24 repo.

The ValueError is getting triggered by /usr/local/lib/python3.10/inspect.py:2271

def _signature_from_builtin(cls, func, skip_bound_arg=True):
    """Private helper function to get signature for
    builtin callables.
    """

    if not _signature_is_builtin(func):
        raise TypeError("{!r} is not a Python builtin "
                        "function".format(func))

    s = getattr(func, "__text_signature__", None)
    if not s:
-        raise ValueError("no signature found for builtin {!r}".format(func))

    return _signature_fromstr(cls, func, s, skip_bound_arg)
jbms commented 2 years ago

Yeah, it would be great if we can add a test case that reproduces the issue. Also, do I understand correctly that even with this PR and the fix from https://github.com/jbms/sphinx-immaterial/pull/142#issuecomment-1216481744, while the build completes, these properties are still not shown with the correct type annotation?

2bndy5 commented 2 years ago

With the suggested fix in https://github.com/jbms/sphinx-immaterial/pull/142#issuecomment-1216481744

using this rST source ```rst .. autoclass:: pyrf24.rf24_network.RF24NetworkHeader :members: to_node, from_node, id, reserved, to_string .. property:: next_id :classmethod: ```

image

As you can see the return types are missing from the properties' signatures, but at least it builds and the properties are shown.

2bndy5 commented 2 years ago

time to move this forward... see https://github.com/jbms/sphinx-immaterial/issues/134#issuecomment-1229419240

I'm ok with squashing this history.

jbms commented 2 years ago

I realized the reason the return types are missing from the properties' signatures is simply that you were using the disable_function_signatures option.

2bndy5 commented 2 years ago

As I developed the test pkg, it also failed (using main branch) without using custom signatures.

jbms commented 2 years ago

Yeah I see that I accidentally introduced a bug previously --- the "unused" fget should have been used.

2bndy5 commented 2 years ago

Yeah I see that I accidentally introduced a bug previously --- the "unused" fget should have been used.

I also tested that, and it didn't break anything.