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

[BUG] sphinx build failures with json parse error #224

Open
fkiraly opened this issue Mar 28, 2024 · 20 comments
Open

[BUG] sphinx build failures with json parse error #224

fkiraly opened this issue Mar 28, 2024 · 20 comments
Labels

Comments

@fkiraly
Copy link
Collaborator

fkiraly commented Mar 28, 2024

The sphinx doc build seems to fail with json parse errors.

I don't think anything was changed that would cause this, it's a bit mysterious.

My suspicion is, based on it being the only json file in the docs afaik, the file docs/source/_static/switcher.json, but all seems ok there?

FYI @yarnabrina, @duydl, in case you have any quick spots.

@fkiraly fkiraly added the bug label Mar 28, 2024
@yarnabrina
Copy link
Member

yarnabrina commented Mar 28, 2024

Can you please share the link where to see this error? Is it in some PR, or on main itself?

@fkiraly
Copy link
Collaborator Author

fkiraly commented Mar 28, 2024

all recent PR, e.g., #222

@duydl
Copy link
Contributor

duydl commented Mar 28, 2024

It is not quite a quick spot but the error is because https://github.com/sktime/skpro/blob/main/docs/source/conf.py#L148

sphinx doesn't get the json file but the whole HTML page so there is parse json error.
On the other hand, I am not sure how builds so far could have passed. json_url must be overwritten somehow: https://github.com/sktime/skpro/blob/main/docs/source/conf.py#L162 .

L148 is edited in #125 to fix #122 but why don't just point it to the _static/switcher.json?

@fkiraly
Copy link
Collaborator Author

fkiraly commented Mar 29, 2024

L148 is edited in #125 to fix #122 but why don't just point it to the _static/switcher.json?

what do you mean by this? Is that not where it aready points to?

@duydl
Copy link
Contributor

duydl commented Mar 29, 2024

No json_url currently points to "https://github.com/sktime/skpro/blob/main/docs/source/_static/switcher.json", which is a website not a json file. It used to point to https://skpro.readthedocs.io/en/latest/_static/switcher.json, which is a json file but changed in #125.

I think you should revert that change. Or you could point it toward https://raw.githubusercontent.com/sktime/skpro/main/docs/source/_static/switcher.json

@fkiraly
Copy link
Collaborator Author

fkiraly commented Mar 29, 2024

oh, I see! Let's try that.

I also remember why I changed #125: The problem was that after manually modifying the json to add a new version, this change would not be visible on the branch, because the json_url pointed to main.

If I revert #125, it possibly fixes failure, but it might introduce the same problem into the release pipeline.

I also wonder, why do we need this? sktime does not seem to have the json_url variable.

@duydl
Copy link
Contributor

duydl commented Mar 29, 2024

It is for the version switcher panel that is theme-specific.
image
It is different than the switcher of RTD. https://pydata-sphinx-theme.readthedocs.io/en/stable/user_guide/version-dropdown.html.

The docs build for tags seem to not be available due to either failures of configs. So I think you could actually remove that panel.

Edit: as you may observe in the newly build docs in PR. The version in the switcher direct to error page.

@fkiraly
Copy link
Collaborator Author

fkiraly commented Mar 29, 2024

hm, what precise change are you suggesting, with "I think you could actually remove that panel."?

@duydl
Copy link
Contributor

duydl commented Mar 29, 2024

I meant removing the json and switcher related configs. Though normally a switcher.json is a minor enhancement for docs. So perhaps you should rebuild the docs for the legacy tags because they are currently not available.

@fkiraly
Copy link
Collaborator Author

fkiraly commented Mar 29, 2024

hm, the switcher dropdown doesn't seem to work for me at all...

@duydl
Copy link
Contributor

duydl commented Mar 29, 2024

hm, the switcher dropdown doesn't seem to work for me at all...

The switcher dropdown for release docs will be empty because probably only builds skipping json work. The one in the PR will have all tag links direct to broken page because they weren't built.

@duydl
Copy link
Contributor

duydl commented Mar 29, 2024

#225 (comment)

The problem was that after manually modifying the json to add a new version, this change would not be visible on the branch, because the json_url pointed to main.

i dont think it would introduce issue. While the changes in switcher not visible in PR or branch, it would still update correctly in release version. Also it have an added benefit that all legacy docs have the dropdown pointing to all available versions.

fkiraly added a commit that referenced this issue Mar 30, 2024
@fkiraly
Copy link
Collaborator Author

fkiraly commented Mar 30, 2024

ok, that makes sense. I reverted to the original state, this will unfortunately lead to release PRs failing.
Any way we can think around that?

@duydl
Copy link
Contributor

duydl commented Mar 31, 2024

Could you point me to an instance of release PR failing due to this change? I would think the currently failed PRs are more likely because their conf.py still had the error before #225 merged.

Also I think you would want to go to
https://readthedocs.org/projects/skpro/versions/
and activate some of the legacy versions included in the switcher.json. Atm the links in the switcher are all dead except for dev one to the latest version

@fkiraly
Copy link
Collaborator Author

fkiraly commented Mar 31, 2024

Thanks, @duydl - I've activated all legacy versions now.

The automation rule for "automatic activate on semver tag" was not enabled, I've enabled this and now in the future it should automatically activate new release tags.

@fkiraly
Copy link
Collaborator Author

fkiraly commented Mar 31, 2024

Could you point me to an instance of release PR failing due to this change?

I'll open one and we can see what happens.

Here, fails as usual:
#228

To note, the code quality failure is not due to linting, it has to do with switcher.js.

@duydl
Copy link
Contributor

duydl commented Mar 31, 2024

Sorry, how did you see it as failed? RTD seemed to build successfully https://readthedocs.org/projects/skpro/builds/23921941/

Also, FYI because most of the tags in the switcher point to version with buggy json_url, they could not be built. Seem like all tags after https://skpro.readthedocs.io/en/v2.1.1/index.html . From v2.1.1 the build is failing.

Edit: I saw the test/code quality CI failure. Isnt it because of trilom/file-changes-action@v1.2.4 action. I am not sure what it does, but is not used in sktime.

@fkiraly
Copy link
Collaborator Author

fkiraly commented Mar 31, 2024

Sorry, how did you see it as failed?

CI in the PR

@fkiraly
Copy link
Collaborator Author

fkiraly commented Mar 31, 2024

Also, FYI because most of the tags in the switcher point to version with buggy json_url, they could not be built. Seem like all tags after https://skpro.readthedocs.io/en/v2.1.1/index.html . From v2.1.1 the build is failing.

Should we rebuild with the URL fixed? E.g., push 2.1.2 where that's the only change?

@duydl
Copy link
Contributor

duydl commented Mar 31, 2024

CI in the PR

RTD seemed to build successfully https://readthedocs.org/projects/skpro/builds/23921941/
I saw the test/code quality CI failure. Isnt it because of trilom/file-changes-action@v1.2.4 action. I am not sure what it does, but is not used in sktime.

Should we rebuild with the URL fixed? E.g., push 2.1.2 where that's the only change?

Or just remove those from the switcher.json . Fixing only the current stable version would be enough imo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants