Provides caching for the linker #498

tristanlatr · 2022-02-18T06:01:00Z

This PR tries to solve #497, it partially succeeds but we also need to cache the whole Documentable summary stan.

It does fixes #478

…ching system to even more efficient and works with options same_page_optimization

codecov · 2022-02-18T06:02:29Z

Codecov Report

Merging #498 (411ae79) into master (c367d21) will increase coverage by 0.16%.
The diff coverage is 96.79%.

@@            Coverage Diff             @@
##           master     #498      +/-   ##
==========================================
+ Coverage   89.90%   90.06%   +0.16%     
==========================================
  Files          35       35              
  Lines        6369     6504     +135     
  Branches     1436     1463      +27     
==========================================
+ Hits         5726     5858     +132     
- Misses        391      392       +1     
- Partials      252      254       +2

Impacted Files	Coverage Δ
pydoctor/epydoc2stan.py	`90.72% <96.55%> (+1.59%)`	⬆️
pydoctor/astbuilder.py	`95.10% <100.00%> (ø)`
pydoctor/model.py	`92.58% <100.00%> (+0.10%)`	⬆️
pydoctor/templatewriter/pages/__init__.py	`84.16% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c367d21...411ae79. Read the comment docs.

tristanlatr · 2022-02-18T07:35:26Z

Note: It does not seem to influence the performance of pydoctor, but many warnings are now displayed only once and not many times! so that's good.

…eed to. Add few docs.

…the warnings

adiroiban · 2022-02-19T09:24:08Z

Thanks for the update.

I am a bit busy these days. if @not-my-profile can review this, that would be great :)

tristanlatr · 2022-02-19T16:46:48Z

pydoctor/model.py

+    @property
+    def docstringlinker(self) -> 'epydoc2stan.DocstringLinker':
+        """
+        Returns an instance of L{epydoc2stan.DocstringLinker} suitable for resolving names
+        in the context of the object scope. 
+        """
+        if self._linker is not None:
+            return self._linker
+        from pydoctor.epydoc2stan import _CachedEpydocLinker
+        self._linker = _CachedEpydocLinker(self)
+        return self._linker


The best would be to avoid this workaround cyclic imports.
In order to do that, we should move all code related to the concrete docstring linkers in a new module: docstringlinker.py. I wonder if this change should happen in this PR though ?

Better for a separate PR, but also add all this info in a code comment :)

Use string annotation in the linker code when possible and add even more docs to the cached linker. Correct english.

tristanlatr · 2022-02-23T17:33:13Z

@adiroiban, I don't think @not-my-profile wants to be involved in pydoctor development. He manifested interest into creating a new doc generator based on mypy internals. So I don't think we should wait for his review.

adiroiban

Hi,Thanks for the update.

If there is no speedup, I am not sure if it's worth it.

Also by not having repeated errors, it might be ok when you run locally, but for CI, I prefer to see all the errors so that I can fix them in one run.

So.. I am now +1 for this change, but you are the maintainer and if you want to have this code, I am ok with that.

Thanks again!

pydoctor/epydoc2stan.py

adiroiban · 2022-02-24T22:33:45Z

pydoctor/epydoc2stan.py

@@ -119,31 +134,46 @@ def look_for_intersphinx(self, name: str) -> Optional[str]:
        return self.obj.system.intersphinx.getLink(name)

    def link_to(self, identifier: str, label: "Flattenable") -> Tag:
+        # :Raises _EpydocLinker.LookupFailed: If the identifier cannot be resolved and self.strict is True.


Maybe have this as docstrig instead of comment?

Actually, I remember. I did not do that not to override the docstirng inherited by ParsedDocstring.
Since this an implementation detail because we're always using the _CachedEpydocLinker, and I did not want to change the pretty well documented docstring that we already have for link_to and link_xref.

adiroiban · 2022-02-24T22:34:27Z

pydoctor/epydoc2stan.py


    def link_xref(self, target: str, label: "Flattenable", lineno: int) -> Tag:
+        # :Raises _EpydocLinker.LookupFailed: If the identifier cannot be resolved and self.strict is True.


Can we have it as docstring?

adiroiban · 2022-02-24T22:35:39Z

pydoctor/model.py

+    @property
+    def docstringlinker(self) -> 'epydoc2stan.DocstringLinker':
+        """
+        Returns an instance of L{epydoc2stan.DocstringLinker} suitable for resolving names
+        in the context of the object scope. 
+        """
+        if self._linker is not None:
+            return self._linker
+        from pydoctor.epydoc2stan import _CachedEpydocLinker
+        self._linker = _CachedEpydocLinker(self)
+        return self._linker


Better for a separate PR, but also add all this info in a code comment :)

pydoctor/test/test_epydoc2stan.py

tristanlatr · 2022-02-24T22:57:20Z

Also by not having repeated errors, it might be ok when you run locally, but for CI, I prefer to see all the errors so that I can fix them in one run.

We are talking about the same errors again and again for the summary table generation, not different errors. Just run

$ wget -O nodes.py https://sourceforge.net/p/docutils/code/HEAD/tree/tags/docutils-0.17.1/docutils/nodes.py?format=raw
$ time pydoctor nodes.py

And you'll see what I mean. See #497

…inker-cache

tristanlatr · 2022-02-25T04:42:58Z

If there is no speedup, I am not sure if it's worth it.

@adiroiban,

I've thought to give up this change and instead implement caching for summaries and docstring stan. But here's the deal: there is not way to simply ignore xref warnings when we call parse_docstring, so warnings are getting reported when we generate stan for the summary, so at least invalid links present in the fist three lines of docstrings will be reported twice even if we cache the docstring and summary stan (and even if we fix #86. Actually we'de need to have both #86 and #421 fixed and compute the summary document from a version with already resolved links in it). So maybe this patch is just the wrong way of dealing with this problem.

I'm unsure what's the best way forward. The class _CachedEpydocLinker is 200 lines of codes and the same in tests. But tests can be recycled anyway. Yes it's a bit of complexity (and multiple invalid link on the same line are reported only once, but it's better than reporting warnings X times the number of summaries we generate for that object. Also if you fix one link in one line in the docstring it's likely that you'll notice the other invalid link that is using the same target on the same line). What I mean is this code can be used until we have fixed #421, #86 and implemented docstring stan caching, then we can remove the _CachedEpydocLinker and simply replace all occurences with _EpydocLinker.

Please tell me what you think, ok I'm maintainer but I do want to fix issue this the right way.

Thanks.

tristanlatr · 2022-03-06T16:26:22Z

Do you have comments @adiroiban ?
Otherwise we would proceed with the proposed plan.

adiroiban · 2022-03-06T16:46:19Z

I am not -1 on this.
And reducing noise in the output is +1.
So I think that this can be merge.

Thanks!

Sorry... I didn't had time to look into pydoctor for twisted usage and why CI doesn't fail an warnings.

…ache

tristanlatr · 2022-03-06T17:45:32Z

Great, thanks!

tristanlatr added 5 commits February 17, 2022 18:09

Create class _CachedEpydocLinker

40fe1c9

Introduce Documentable.docstringlinker

7f0e1b0

Use the cached version of the linker all the time and refactor the ca…

783b7c7

…ching system to even more efficient and works with options same_page_optimization

docs

157e16f

Fix bug

1076985

tristanlatr marked this pull request as draft February 18, 2022 06:01

tristanlatr added 3 commits February 18, 2022 01:26

Fix mypy

98b7ed8

docs

e16e4a1

add a test for the linker

0721175

tristanlatr added 3 commits February 18, 2022 03:40

Fix bug and add test

cbd4ab5

add docs

3464297

Adjust linker to cache even more, do not clone tags when they don't n…

401eccc

…eed to. Add few docs.

tristanlatr marked this pull request as ready for review February 18, 2022 17:45

tristanlatr added 2 commits February 18, 2022 12:46

Typo

56163ce

Get cache only once

69347a7

tristanlatr requested a review from adiroiban February 18, 2022 17:56

tristanlatr marked this pull request as draft February 18, 2022 19:27

tristanlatr added 5 commits February 18, 2022 14:49

Add more tests and discover that there is still an little issue with …

87cec24

…the warnings

Fix issue with warnings

ab00164

Avoid changing _EpydocLinker interface and add few docs

f56a0b7

add docs

6c759f1

Fix mypy

d564b1a

tristanlatr marked this pull request as ready for review February 18, 2022 23:29

tristanlatr added 2 commits February 18, 2022 18:29

Remove unused import

bd2e2b7

Refactor to minimize code duplication

158bb2e

tristanlatr commented Feb 19, 2022

View reviewed changes

Bit of refactoring.

8a9a9b0

Use string annotation in the linker code when possible and add even more docs to the cached linker. Correct english.

Merge branch 'master' into linker-cache

04e5b22

adiroiban approved these changes Feb 24, 2022

View reviewed changes

Fix format

11f0f4a

tristanlatr mentioned this pull request Feb 25, 2022

Isolate docstring linker code in it's own module #507

Closed

tristanlatr added 2 commits February 24, 2022 20:48

Merge branch 'linker-cache' of github.com:tristanlatr/pydoctor into l…

e01cbf1

…inker-cache

Add code comment

cbe1b85

tristanlatr mentioned this pull request Feb 25, 2022

Pydoctor is slow if there are many subclasses (caching for summary stan) #497

Closed

tristanlatr added 2 commits March 6, 2022 12:28

Merge commit 'c367d216e7a16daa18b1d2216d0cb1976cbf7bdf' into linker-c…

3f0c6df

…ache

Add changelog entry

411ae79

tristanlatr merged commit 1210f94 into twisted:master Mar 6, 2022

tristanlatr mentioned this pull request Mar 6, 2022

Links in docstring summary break when the summary is included on another page #478

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provides caching for the linker #498

Provides caching for the linker #498

tristanlatr commented Feb 18, 2022 •

edited

Loading

codecov bot commented Feb 18, 2022 •

edited

Loading

tristanlatr commented Feb 18, 2022

adiroiban commented Feb 19, 2022

tristanlatr Feb 19, 2022

adiroiban Feb 24, 2022

tristanlatr commented Feb 23, 2022

adiroiban left a comment

adiroiban Feb 24, 2022

tristanlatr Feb 24, 2022

tristanlatr Feb 24, 2022 •

edited

Loading

adiroiban Feb 24, 2022

adiroiban Feb 24, 2022

tristanlatr commented Feb 24, 2022

tristanlatr commented Feb 25, 2022

tristanlatr commented Mar 6, 2022

adiroiban commented Mar 6, 2022

tristanlatr commented Mar 6, 2022


		def link_xref(self, target: str, label: "Flattenable", lineno: int) -> Tag:
		# :Raises _EpydocLinker.LookupFailed: If the identifier cannot be resolved and self.strict is True.

Provides caching for the linker #498

Provides caching for the linker #498

Conversation

tristanlatr commented Feb 18, 2022 • edited Loading

codecov bot commented Feb 18, 2022 • edited Loading

Codecov Report

tristanlatr commented Feb 18, 2022

adiroiban commented Feb 19, 2022

tristanlatr Feb 19, 2022

Choose a reason for hiding this comment

adiroiban Feb 24, 2022

Choose a reason for hiding this comment

tristanlatr commented Feb 23, 2022

adiroiban left a comment

Choose a reason for hiding this comment

adiroiban Feb 24, 2022

Choose a reason for hiding this comment

tristanlatr Feb 24, 2022

Choose a reason for hiding this comment

tristanlatr Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

adiroiban Feb 24, 2022

Choose a reason for hiding this comment

adiroiban Feb 24, 2022

Choose a reason for hiding this comment

tristanlatr commented Feb 24, 2022

tristanlatr commented Feb 25, 2022

tristanlatr commented Mar 6, 2022

adiroiban commented Mar 6, 2022

tristanlatr commented Mar 6, 2022

tristanlatr commented Feb 18, 2022 •

edited

Loading

codecov bot commented Feb 18, 2022 •

edited

Loading

tristanlatr Feb 24, 2022 •

edited

Loading