add headings to weights table.

[abhi-glitchhg]

Related to #6126 and #6131

Here is my attempt.

[NicolasHug]

This creates the following error in the doc build:

Warning, treated as error:
/home/circleci/project/docs/source/models.rst:263:Title level inconsistent:

Table of all available quantized classification weights
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
make: *** [Makefile:42: html] Error 2

[NicolasHug]

Thanks for the PR @abhi-glitchhg but I think the current solution adds titles below the tables: https://output.circle-artifacts.com/output/job/7b9d5b75-850e-4421-acd7-d4f7b6fa314f/artifacts/0/docs/models.html#classification-weights-table

[NicolasHug]

I think the problem is just reversed now, the title is above the table but we still have duplicated titles which is redundant:

Any chance we can tell the table to use the first title as its title in the search?

[abhi-glitchhg]

Any chance we can tell the table to use the first title as its title in the search?

I think I can do that.

[NicolasHug]

I would prefer not to change the current heading levels and keep what we currently have in https://pytorch.org/vision/main/models.html

[datumbox]

@NicolasHug True but is this possible? Aka if you add headings aren't those going to automatically be appeared to the heading levels?

[NicolasHug]

Yes, which is why ideally we would find a way to tell the table to just use its section title as title #6139 (comment)

This way we don't have to add another heading level.

[abhi-glitchhg]

I would prefer not to change the current heading levels and keep what we currently have in https://pytorch.org/vision/main/models.html

Thanks! Then I think I am done, please check the docs and let me know.

[datumbox]

@abhi-glitchhg Push!!! :)

[NicolasHug]

IIUC this is the same as before but instead of writing these lines in the .rst file (as preferred), we're now generating it here and writing it in the table files.

Could you explain what the difference is, and why it "works"?

[abhi-glitchhg]

IIUC this is the same as before, but instead of writing these lines in the .rst file (as preferred), we're now generating it here and writing it in the table files.

Yes, you are right!

So there was no title/headings in the generated table rst files, that is why we were getting results like below.
(notice the <no title>)

So to solve this, we needed to add the titles in the generated table files and not manually write them in models.rst.
And the descriptions for the tables should be written after the headings. So there was no choice but to add the description in the generated table rst.

So, I have shifted the title and description of tables from models.rst to generated table rst files. Otherwise, there is no difference.

[abhi-glitchhg]

@NicolasHug, any updates on this?

[NicolasHug]

Hi @abhi-glitchhg , sorry for the late reply

I'm a little uncomfortable with this solution because it makes our solution slightly more complex and somewhat hides the structure of the models.rst file, which now also depends on the auto-generation code in conf.py. On top of that it's not really clear why this works while the our original solution doesn't.

It feels like we're patching a limitation of sphinx's search by working around it, without addressing the actual core of the issue. Did we figure out why writing the title within the file makes the search render better?

BTW, the search still looks like this:

which is better because we have the title, but it still looks broken. Considering how much time we have spent on this already (especially you!), I wonder if it's worth continuing trying to fix this. It seems to me like a benign issue to begin with.

[abhi-glitchhg]

It feels like we're patching a limitation of sphinx's search by working around it without addressing the actual core of the issue. Did we figure out why writing the title within the file makes the search render better?

Yeah maybe!

which is better because we have the title, but it still looks broken.

Agree! Closing this pr as it doesn't properly solve the issue.

[abhi-glitchhg]

Maybe someone with a good understanding of the sphinx theme could have a look at this!

maybe @ain-soph (sorry for shameless tagging, i really liked how you modified the theme for your project) If you have spare time ;-;

[datumbox]

@abhi-glitchhg I am very sorry that none of our team got back to you. Many of us had our annual leaves this period and your PR fell through the cracks. :( Let me ping Nicolas tomorrow internally and try to get you some feedback.

[abhi-glitchhg]

No problem at all. I know how busy you all must be and it's very much possible that somethings might get unnoticed. I appreciate your support! Thanks 🙏.

…

On Tue, Aug 9, 2022, 00:48 Vasilis Vryniotis ***@***.***> wrote: @abhi-glitchhg <https://github.com/abhi-glitchhg> I am very sorry that none of our team got back to you. Many of us had our annual leaves this period and your PR fell through the cracks. :( Let me ping Nicolas tomorrow internally and try to get you some feedback. — Reply to this email directly, view it on GitHub <#6139 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ARLRQF5ONLIFD4YTNQVKT7LVYFMO7ANCNFSM5YHKPNIA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- The information contained in this electronic communication is intended solely for the individual(s) or entity to which it is addressed. It may contain proprietary, confidential and/or legally privileged information. Any review, retransmission, dissemination, printing, copying or other use of, or taking any action in reliance on the contents of this information by person(s) or entities other than the intended recipient is strictly prohibited and may be unlawful. If you have received this communication in error, please notify us by responding to this email or telephone and immediately and permanently delete all copies of this message and any attachments from your system(s). The contents of this message do not necessarily represent the views or policies of BITS Pilani.

[ain-soph]

@abhi-glitchhg Thanks for getting me involved. I just went through the PR, and it seems the current problem is that we prefer table context/titles coded in .rst files rather than conf.py. (I agree this as well. Furthermore, it seems strange to me that we call this generate_weights_table method to create tables in conf.py since this file should only contains configurations and utility functions.)

I think a better idea might be to register a new sphinx directive to parse tables in setup()? It shall be doable in conf.py without touching theme package. Later we can apply the new directive in .rst files and pass content-related arguments there. (The same process as autosummary)

In current architecture, it's a python method rather than a sphinx directive, which forces it to be called in conf.py.

[NicolasHug]

I think a better idea might be to register a new sphinx domain to parse tables in setup()? It shall be doable in conf.py without touching theme package. Later we can apply the new domain in .rst files and pass content-related arguments there. (The same process as autosummary)

Thanks for your insights @ain-soph . I'm happy to refactor our implementation to something more standard / canonical, as long as it's not too complex or overkill. Out of curiosity, do you believe this would solve our "table title in search rendering issue" ?

[ain-soph]

@NicolasHug

Here is how to set the table title (very simple):

I see you already tried that in previous PR.

https://stackoverflow.com/questions/53191945/how-can-i-add-a-caption-to-a-restructuredtext-table

And for the <no-title> shown in the search page, that’s actually because section name of the generated rst page is not set. (Not related to the table directive name. Therefore, @NicolasHug ‘s previous PR doesn’t solve the no title issue.)

@abhi-glitchhg has already solved that correctly by setting the section name of the generated page.

<section name>
—————————————————————

[abhi-glitchhg]

Ok ill give this one more try!
Thanks a lot, @ain-soph for the directions!

[ain-soph]

@abhi-glitchhg Nah, I think adding the table title won’t help anything. You have already solved the no title issue in your previous commits.

I think the current issue from maintainers is that: those content-related texts are put into conf.py, which violates the initial purpose of this configuration file. (Actually they call this generation method in this file already violated the convention.)

My suggestion is to implement a new small plugin which contains a new sphinx directive and register it in conf.py. The plug-in will be highly similar to autosummary that preprocess your auto rst files and generate the real table rst files.

[abhi-glitchhg]

oh I misunderstood your previous message. Thanks for pointing it out.

My suggestion is to implement a new small plugin which contains a new sphinx directive and register it in conf.py. The plug-in will be highly similar to autosummary that preprocess your auto rst files and generate the real table rst files.

I do not have much knowledge of implementing this, So closing this pr [I promise wont open this pr again 🤭 ]

If anyone wants to resolve this, feel free to supersede me. Thanks.

[ain-soph]

I shall be available to work on this after finishing the issue at #6224

Maybe 1 month later?