skpro icon indicating copy to clipboard operation
skpro copied to clipboard

[BUG] sphinx build failures with json parse error

Open fkiraly opened this issue 1 year ago • 20 comments

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 avatar Mar 28 '24 14:03 fkiraly

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

yarnabrina avatar Mar 28 '24 14:03 yarnabrina

all recent PR, e.g., https://github.com/sktime/skpro/pull/222

fkiraly avatar Mar 28 '24 17:03 fkiraly

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?

duydl avatar Mar 28 '24 23:03 duydl

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?

fkiraly avatar Mar 29 '24 11:03 fkiraly

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

duydl avatar Mar 29 '24 12:03 duydl

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.

fkiraly avatar Mar 29 '24 12:03 fkiraly

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.

duydl avatar Mar 29 '24 12:03 duydl

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

fkiraly avatar Mar 29 '24 12:03 fkiraly

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.

duydl avatar Mar 29 '24 12:03 duydl

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

fkiraly avatar Mar 29 '24 14:03 fkiraly

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 avatar Mar 29 '24 15:03 duydl

https://github.com/sktime/skpro/pull/225#issuecomment-2027339709

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.

duydl avatar Mar 29 '24 15:03 duydl

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?

fkiraly avatar Mar 30 '24 12:03 fkiraly

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

duydl avatar Mar 31 '24 08:03 duydl

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 avatar Mar 31 '24 12:03 fkiraly

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: https://github.com/sktime/skpro/pull/228

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

fkiraly avatar Mar 31 '24 12:03 fkiraly

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/[email protected] action. I am not sure what it does, but is not used in sktime.

duydl avatar Mar 31 '24 13:03 duydl

Sorry, how did you see it as failed?

CI in the PR

fkiraly avatar Mar 31 '24 13:03 fkiraly

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?

fkiraly avatar Mar 31 '24 13:03 fkiraly

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/[email protected] 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.

duydl avatar Mar 31 '24 13:03 duydl