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

add the start of the swagger documentation for the HELICS REST API #2289

Merged
merged 15 commits into from
May 1, 2022

Conversation

phlptp
Copy link
Member

@phlptp phlptp commented Feb 22, 2022

Summary

If merged this pull request will add swgger files for the REST API. There is more to come so this is main just the start and to start working with the files to begin the process of how to display and host them.

@phlptp phlptp added the documentation Issues pertaining to missing documentation or errors in documentation label Feb 22, 2022
@phlptp
Copy link
Member Author

phlptp commented Mar 14, 2022

@nightlark @trevorhardy @kdheepak
Can you take a look at this, I think this is the extent of what I can get done for the 3.2 release. I expect it will be updated as revised in later updates

Copy link
Contributor

@trevorhardy trevorhardy left a comment

Choose a reason for hiding this comment

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

It would be great to come up with a better way to view the swagger API stuff.

description: the HELICS core type to use when creating a broker
CoreType:
type: string
description: same as type
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what "same as type" means.

@@ -0,0 +1,60 @@
title: FederateInfo
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to be a beast, right? So many options to support or do we have a better way?

Copy link
Member Author

Choose a reason for hiding this comment

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

All the specific options that are not flags are listed already. I will probably expand this later to include the flags.
This is eventually meant to document the json configuration files but not fully there yet.

@nightlark
Copy link
Member

Here's a sphinx extension for redoc:
https://sphinxcontrib-redoc.readthedocs.io/en/stable/

And their demo of a generated documentation page (on readthedocs):
https://sphinxcontrib-redoc.readthedocs.io/en/stable/api/github/#

RTD conf.py script for their example build:
https://github.com/sphinx-contrib/redoc/blob/master/docs/conf.py

@phlptp
Copy link
Member Author

phlptp commented Mar 24, 2022

Should we try to make those changes for swagger working with sphinx in this PR or a subsequent one?

@trevorhardy
Copy link
Contributor

trevorhardy commented Mar 24, 2022

@nightlark, do you want to take a crack at this or should I? I can probably put a little bit of time into it in the next day or two if you can't/don't.

That is, @phlptp, I think it should be a small lift. I wouldn't hold up any release for this unless it is important.

@phlptp
Copy link
Member Author

phlptp commented Mar 24, 2022

@trevorhardy if you want to take a crack at it go ahead. It is going to be at least a few days yet before a release so we can reassess once a release is immanent if we are not making much progress.

@nightlark
Copy link
Member

I think we can merge this PR and do the RTD changes as a separate PR.

docs/requirements.txt Outdated Show resolved Hide resolved
redoc = [
{
"name": "HELICS REST API",
"page": "references/api-reference/rest_queries_api",
Copy link
Member

@nightlark nightlark Mar 25, 2022

Choose a reason for hiding this comment

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

I think it would be better to generate this in the "top-level" swagger folder, similar to how the generated doxygen stuff is under a doxygen folder rather than nested under references/api-references.

Suggested change
"page": "references/api-reference/rest_queries_api",
"page": "swagger",

And then also add a directive to RTD saying to include the contents of the swagger folder so that all the yaml/json specs and model files are copied to the website. Using a separate top-level "swagger" folder for the generated swagger docs should also help prevent accidentally including all of the raw markdown folders in the published RTD site.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nightlark, If you think you know how to get this working, be my guest. I'm kind of guessing at how to get all the pathing right.

Copy link
Member

Choose a reason for hiding this comment

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

I need to look into the redoc settings, but I think the original pathing and generated swagger files were fine. The two issues I think for why it didn't work are:

  • RTD conf.py needs to specify that the swagger folder with model yaml files be included in the deployed site; embed is set to True for redoc, so it stores a copy of queries.yaml in the generated api page but the stored copy has paths referring to the other yaml files which aren't included on the RTD site
  • embed should probably be left as the default value False, making so that the relative paths in queries.yaml point to the right place; when queries.yaml is embedded it looks like the generated in an arbitrary folder that breaks the relative paths

I'll see if I can try to get it working later today or tonight, if it isn't already working.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll rollback to the an earlier version and try just turning embed off.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do think the swagger folder needs to end up in the api-reference folder but we can tweak that once we get things working.

Copy link
Member

@nightlark nightlark Mar 25, 2022

Choose a reason for hiding this comment

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

The swagger folder doesn't need to end up in api-reference (we'll treat it the same as we do the doxygen folder).

https://github.com/GMLC-TDC/HELICS/blob/swagger/docs/conf.py#L104
I think changing this line from:

html_extra_path = [os.path.abspath(os.path.join(doxygen_build_dir, "docs", "html"))]

to:

html_extra_path = ["swagger", os.path.abspath(os.path.join(doxygen_build_dir, "docs", "html"))]

should make so that the swagger folder gets included in the deployed site.

Copy link
Member

@nightlark nightlark Mar 25, 2022

Choose a reason for hiding this comment

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

From my (current) understanding of how redoc works, it looks like reverting the changes back to commit ded62ea, making the html_extra_path change, and turning embed off could work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, this link still returns a 404. I'll see if I come up with any other great ideas.

Copy link
Member

@nightlark nightlark Mar 25, 2022

Choose a reason for hiding this comment

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

Yeah -- it looks like https://docs.helics.org/en/swagger/swagger/models/Filter.yaml doesn't exist, so I think it didn't copy the folder. Might be that the absolute path to the swagger folder needs to be given instead to make RTD include it... or relative paths are to something like the repository root instead rather than the docs subfolder.

Another interesting observation from your link, it looks like turning off embed makes it look for the specs in some _specs folder. Kinda strange.

@phlptp
Copy link
Member Author

phlptp commented Mar 31, 2022

@nightlark @trevorhardy where are we at with this, I know you were working on figuring out how to get it built into the readthedocs? any Luck?

@trevorhardy
Copy link
Contributor

@phlptp I haven't been able to get it working and haven't touched it since last week. I was hoping @nightlark might work his magic...

@nightlark
Copy link
Member

I'm trying to get a local build of the swagger docs working to figure out what all the files it decides to generate are. Still trying to figure out why I'm seeing so many build errors, with some Jupyter notebooks sphinx extension failing -- I don't think we're even using any Jupyter notebook stuff on RTD.

@nightlark
Copy link
Member

I got a local build working (virtualenv folder was in the docs folder, so it died when trying to parse some random markdown/rst files included in pip packages).

So far, the smaller issue is that including the swagger folder is just copying the contents of swagger into the root html output folder; I think I can work around that by creating an doc_extras folder and move swagger into that.

The bigger issue is the sphinx redoc extension -- it has no option to tell it not to move the included OpenAPI/swagger spec file when that is provided as a relative path; and any movement of the API spec file relative to the other swagger files will break relative references. The three possible fixes I'm seeing are:

  • making a patched version of the sphinx redoc extension with options to give more control over placement of the final spec file; modified version of the extension would need to be uploaded to PyPI as it seems the original maintainer isn't able to maintain it anymore: Ordering of endpoints is random with the use of yaml library : switch to oyaml sphinx-contrib/redoc#31 (comment))
  • turning on the embed option and trying to coerce it into placing the final html file in the exact folder as the rest of the swagger model files
  • getting a JSON file similar to swagger/references/web-server.json to be used instead of queries.yaml with a bunch of referenced yaml files, since the JSON file has all the needed info with no external references (@phlptp is there an option in the tool you were using that can do this?); with the redoc embed option turned on, this should just work, so I think it would be the preferred option

@phlptp
Copy link
Member Author

phlptp commented Mar 31, 2022

we can probably convert queries.yaml to a json file, but it would still make reference to all the model files. Since those model files are used in multiple locations, it would be bad software practice to try to mash everything into a single file, so I don't want to do that. And in that case I am not sure converting to a json would make much difference.

@nightlark
Copy link
Member

nightlark commented Mar 31, 2022

I just checked the web-server.json file -- it does have relative paths, so yea, that won't work either.

It would be much easier if sphinxcontrib-redoc had separate options to specify the "root" folder to copy containing all the swagger model files, and an option to specify the spec to load as the entrypoint for the doc page.

Copy link
Member

@nightlark nightlark left a comment

Choose a reason for hiding this comment

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

Swagger docs build using redoc on RTD -- link to the generated page for the REST Queries API is on the page under References->API References. I think there are some issues related to the yaml models, which result in warnings being shown.

The build for this PR is at: https://helics--2289.org.readthedocs.build/en/2289/references/api-reference/rest_queries_api.html

@phlptp phlptp merged commit 8a4d20e into develop May 1, 2022
@phlptp phlptp deleted the swagger branch May 1, 2022 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issues pertaining to missing documentation or errors in documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants