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 json schemas to docs #1314

Closed

Conversation

mvpatel2000
Copy link
Contributor

@mvpatel2000 mvpatel2000 commented Jul 25, 2022

Adds json schemas to docs. Adds a test to make sure the generated files are the most recent version

@mvpatel2000
Copy link
Contributor Author

Still need to verify docs generate properly. Tagging for review for discussion of questions

Copy link
Contributor

@ravi-mosaicml ravi-mosaicml left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! Wow, this will be powerful.

Discussed offline, but can we use jsonschema $defs or $refs to reduce the file size of the json schemas?

Afterwards, I'll take a look on how this shows on the docs site.

@mvpatel2000 mvpatel2000 requested a review from a team as a code owner July 26, 2022 01:50
@moinnadeem
Copy link
Contributor

Is this related to CO-669, CO-670, or CO-671? @ravi-mosaicml @mvpatel2000

@mvpatel2000
Copy link
Contributor Author

Is this related to CO-669, CO-670, or CO-671? @ravi-mosaicml @mvpatel2000

CO-671. The first two are done, though there's some changes we had to make as a result of this PR here.

Copy link
Contributor

@ravi-mosaicml ravi-mosaicml left a comment

Choose a reason for hiding this comment

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

Overall this is awesome -- see comments.

In the JSONSchema generation (likely in the YAHP PR), one more area to make it more compact:

is it possible to specify properties and patternPropertiesin the same object? Trying to reduce some duplication -- e.g. this seems redundant

{
                    "additionalProperties": false,
                    "properties": {
                        "deeplabv3": {
                            "$ref": "#/$defs/DeepLabV3Hparams"
                        }
                    },
                    "type": "object"
                },
                {
                    "additionalProperties": false,
                    "patternProperties": {
                        "^deeplabv3\\+": {
                            "$ref": "#/$defs/DeepLabV3Hparams"
                        }
                    },
                    "type": "object"
                },

Before approving, would like to fix the docs build + bump the new yahp version and upgrade yahp in the dependencies.

@@ -262,6 +262,8 @@ class TrainerHparams(hp.Hparams):
grad_clip_norm (float, optional): See :class:`.Trainer`.

profiler (ProfilerHparams, optional): Profiler hyperparameters.

.. jsonschema:: ../json_schemas/trainer_hparams.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of generating a new jsonschema for each hparams class, and linking in the docs to that file, can we link the the definition of that hparams class within the experiment hparams json schema? The experiment hparams is the "root", and all possible hparams should be used within it. The JSONSchema plugin supports JSONPointer notation.

For example:

Suggested change
.. jsonschema:: ../json_schemas/trainer_hparams.json
.. jsonschema:: ../json_schemas/experiment_hparams.json#/defs/trainer_hparams

This should help reduce the duplication, and make the docs site more accessible (since there will be just one jsonschema entry)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're robbing me of my 25k lines committed though :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re offline discussion, we actually do need all the files to enable autocomplete

@@ -0,0 +1,63 @@
# Copyright 2022 MosaicML Composer authors
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you add a shebang? #!/usr/bin/env python3 should suffice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there a reason why? It looks like no other file has this?

@mvpatel2000
Copy link
Contributor Author

mvpatel2000 commented Jul 27, 2022

Note: All glue related yaml files are failing because of this. The rest are failing because we still haven't bumped yahp version

@mvpatel2000
Copy link
Contributor Author

this is not really human readable sad. Discussing alternatives offline

@mvpatel2000
Copy link
Contributor Author

Closing this PR -- after a lot of offline discussion, we've decided not to insert JSON Schema into the docs. We'll instead do a skinny PR with just the test infra.

@mvpatel2000 mvpatel2000 closed this Aug 5, 2022
@mvpatel2000 mvpatel2000 deleted the mvpatel2000/add-json-schemas branch August 5, 2022 20:47
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.

3 participants