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

Remove unnecessary html_theme_path option #573

Merged
merged 3 commits into from
Mar 21, 2023

Conversation

Eric-Arellano
Copy link
Contributor

See qiskit-community/qiskit-finance#244 for the motivation.

I built the docs locally on main vs this branch and could find no differences between the two, as expected.

Copy link
Collaborator

@adekusar-drl adekusar-drl left a comment

Choose a reason for hiding this comment

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

Seems like the PR introduced the same problem as in optimization here: qiskit-community/qiskit-optimization#475. Spell checker fails on the class names in the warning messages. I'm not sure I want to add all that class names to the dictionary.

@woodsp-ibm
Copy link
Member

Since #571 passed yesterday I am re-running that to see if the failure might be related to a change in a library or dependency. I tried looking at versions but the spelling checker etc seem the same versions.

@adekusar-drl
Copy link
Collaborator

Since #571 passed yesterday I am re-running that to see if the failure might be related to a change in a library or dependency. I tried looking at versions but the spelling checker etc seem the same versions.

CI have run twice on this PR with the same results.

@woodsp-ibm
Copy link
Member

It has to be something that changed very recently but not what was done by this PR. The sparse TorchConnector PR #571 that passed yesterday ok, fails today the same way as this PR now having just re-run it.

@adekusar-drl
Copy link
Collaborator

Oh, I see now. Then you are right, something has been changed very recently.

@woodsp-ibm
Copy link
Member

woodsp-ibm commented Mar 16, 2023

It seems to be the class names at the end of the long class paths in the deprecation messages that it didn't pull off and check before. Not sure why now. Would any alteration to the message , say putting the whole path say in singles/double quotes stop it as a alternative to adding it to spell checker - something we'd have to try I guess if we don't want to add those class names to the dict. We can choose to fix the spelling, however we do that, in a separate PR.

@woodsp-ibm
Copy link
Member

I was wondering why Nature did not hit this since it has a lot of deprecations. With QuantumKernel here that is failing the class names are directly in the message (string) here versus in params of a decorator. Maybe both can go in the new name string parameter of a decorator rather than as currently done as a different option.

@adekusar-drl
Copy link
Collaborator

At the first glance, I have not spotted the difference between two setups: when CI failed and when CI checks were successful. Looks like the library versions are identical.

@woodsp-ibm
Copy link
Member

woodsp-ibm commented Mar 16, 2023

Now the failure here with the spelling in QuantumKernel, I noticed it's using a Terra decorator - that was just changed and now includes the deprecation message into the doctring which the spelling ends up seeing. So its not its presence in the source exactly I think more that it gets injected into the docstring which is checked. You can see the result in the html built:

image

We put the full path without the :class: in the message as that will not get resolved by Sphinx - in this case it could be but that would mean parsing/altering the message which maybe is not desirable.

@adekusar-drl
Copy link
Collaborator

Perhaps, this commit Qiskit/qiskit#9790 introduced that behavior. I will check it out later.

@woodsp-ibm
Copy link
Member

Yes, Eric has been working on improving the decorators. The problem in Optimization is related but a bit different. Hopefully we can figure what the best way forward is given that it was recognized it would be very helpful for the deprecation info to be in the docs too for an end user and for it to be automated,

@woodsp-ibm woodsp-ibm added this to the 0.6.0 milestone Mar 21, 2023
@coveralls
Copy link

Pull Request Test Coverage Report for Build 4479276456

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 90.892%

Totals Coverage Status
Change from base Build 4479272260: 0%
Covered Lines: 3423
Relevant Lines: 3766

💛 - Coveralls

@adekusar-drl adekusar-drl merged commit 3a869ae into qiskit-community:main Mar 21, 2023
@Eric-Arellano Eric-Arellano deleted the rm-theme-path branch March 25, 2023 12:28
oscar-wallis pushed a commit that referenced this pull request Feb 16, 2024
Co-authored-by: Anton Dekusar <62334182+adekusar-drl@users.noreply.github.com>
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

4 participants