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

Missing 1.0 docs #701

Merged
merged 22 commits into from
Mar 14, 2022
Merged

Missing 1.0 docs #701

merged 22 commits into from
Mar 14, 2022

Conversation

JackUrb
Copy link
Contributor

@JackUrb JackUrb commented Mar 10, 2022

Overview

As one of the last steps for Mephisto 1.0, this PR runs through some remaining docs pages that need content:

To come shortly after 1.0 would be the following docs:

For these de-prioritized docs, what would be the appropriate messaging to leave on the pages?

For autogenerated python docs, I've used pdoc and dropped the static docs into static. This included a number of changes to __init__.py files to autoinclude readmes. It also moved a lot of scripts behind __main__ blocks, as these were being executed during the doc build process.

Example API docs:
Screen Shot 2022-03-11 at 1 58 42 PM

Note: the ergonomics aren't great as this triggers a new tab load instead of being in the same tab, but I don't have any easy fix for this just yet.

@JackUrb JackUrb requested a review from pringshia March 10, 2022 22:56
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 10, 2022
@JackUrb JackUrb mentioned this pull request Mar 10, 2022
13 tasks
Base automatically changed from boilerplate-cutdown to dev-1.0 March 10, 2022 23:13
@codecov-commenter
Copy link

codecov-commenter commented Mar 11, 2022

Codecov Report

Merging #701 (502c928) into dev-1.0 (d201a75) will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           dev-1.0     #701      +/-   ##
===========================================
- Coverage    64.17%   64.11%   -0.06%     
===========================================
  Files           95      107      +12     
  Lines         9169     9199      +30     
===========================================
+ Hits          5884     5898      +14     
- Misses        3285     3301      +16     
Impacted Files Coverage Δ
mephisto/__init__.py 86.66% <ø> (+2.05%) ⬆️
mephisto/abstractions/__init__.py 100.00% <ø> (ø)
mephisto/abstractions/architects/__init__.py 100.00% <ø> (ø)
...ephisto/abstractions/architects/router/__init__.py 100.00% <ø> (ø)
mephisto/abstractions/blueprints/__init__.py 100.00% <ø> (ø)
...histo/abstractions/blueprints/abstract/__init__.py 100.00% <ø> (ø)
...lueprints/abstract/static_task/static_blueprint.py 40.81% <ø> (ø)
...ractions/blueprints/mixins/screen_task_required.py 76.84% <ø> (+0.49%) ⬆️
...to/abstractions/blueprints/mixins/use_gold_unit.py 31.29% <ø> (ø)
...sto/abstractions/blueprints/mock/mock_blueprint.py 87.50% <ø> (ø)
... and 41 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d201a75...502c928. Read the comment docs.

@pringshia
Copy link
Contributor

For the de-prioritized docs, I would suggest either:

  • leaving a short one-or-two liner giving a high level summary (that we will expand on later),
  • or removing altogether temporarily (as opposed to leaving a publicly visible TODO or Coming Soon).

python run_task.py mephisto/architect=heroku mephisto/provider=mturk_sandbox mephisto.provider.requester_name=MY_REQUESTER
```

Instead you can move these common configurations into a file in your `~/.mephisto/hydra_configs/profile` dir.
Copy link
Contributor

@pringshia pringshia Mar 12, 2022

Choose a reason for hiding this comment

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

Maybe a provocative question, but is there any reason to have hydra_configs as part of the path? In the example folders we also have things like parlai_chat_task_demo/hydra_configs/conf/.... always as a nested folder. seems unnecessary to have the double nesting. Perhaps hydra_config is part of the defaults but maybe we can use something straightforward here? this should also be easier now that we control the decorator.

Perhaps just conf/, e.g ~/.mephisto/conf/ that houses the profiles? or ~/.mephisto/profiles/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a hydra requirement, you need to point to a root folder, and then all sub folders of that folder are possible configuration options that can be set. As such it needs its own folder, or else hydra will consider all of the things present possible arguments. Agreed that it's not ergonomic, but I'm not sure how else we do it.

@pringshia
Copy link
Contributor

It would be nice to have better pdoc integration w/ Docusaurus, like this example. Seems like this is the template they use: https://github.com/h2oai/wave/blob/master/py/docs/templates/text.mako

and this is how it's built via the --template-dir command:

https://github.com/h2oai/wave/blob/master/py/Makefile#L25-L31

I can take a look at wiring this up this weekend.

@pringshia
Copy link
Contributor

pringshia commented Mar 12, 2022

So I see:

"docs:static": "pdoc mephisto --logo https://mephisto.ai/img/logo.svg -o docs/web/static/python_api/ -t docs/web/pdoc_src"

in the root package.json but don't see the dependency specified anywhere? Were you installing pdoc through npm or python?

@JackUrb
Copy link
Contributor Author

JackUrb commented Mar 12, 2022

It would be nice to have better pdoc integration w/ Docusaurus, like this example. Seems like this is the template they use: https://github.com/h2oai/wave/blob/master/py/docs/templates/text.mako

and this is how it's built via the --template-dir command:

https://github.com/h2oai/wave/blob/master/py/Makefile#L25-L31

I can take a look at wiring this up this weekend.

I spent about an hour or two walking through this rabbit hole - this is a super nice experience, but pdoc doesn't support output to markdown. pdoc3 does, but is a fork of the original that isn't as well maintained and breaks down doing the conversion. I tried to make the markdown versions in 2-3 different ways using pdoc3, but each failed around a number of issues around typing documentation generating items of the form <type> which the html->markdown software totally dies on. If you want to submit a fix to pdoc3 at some point around silently dropping failed tags though I'm sure we can change back over, but I don't think it's worth your time compared to this.

@JackUrb
Copy link
Contributor Author

JackUrb commented Mar 12, 2022

So I see:

"docs:static": "pdoc mephisto --logo https://mephisto.ai/img/logo.svg -o docs/web/static/python_api/ -t docs/web/pdoc_src"

in the root package.json but don't see the dependency specified anywhere? Were you installing pdoc through npm or python?

It's installed through python, I didn't know the right place to put this otherwise if our other scripts are here.

@pringshia
Copy link
Contributor

Saw your message too late, that's a few hours I'll never get back 😆 Man what a mess trying to use pdoc3.

@pringshia
Copy link
Contributor

pringshia commented Mar 12, 2022

It's installed through python, I didn't know the right place to put this otherwise if our other scripts are here.

Unsure whether dev dependencies like this belong in the requirements.txt. Can't think of any other place so leaning towards yes? Or should we have something like dev-requirements.txt. Is there precedence?

Edit: https://realpython.com/lessons/production-vs-development-dependencies/ mentions a requirements_dev.txt

@pringshia
Copy link
Contributor

this triggers a new tab load

FYI: facebook/docusaurus#3309 (comment)

@pringshia pringshia mentioned this pull request Mar 13, 2022
3 tasks
@mhils
Copy link

mhils commented Mar 14, 2022

Hi folks! I found this thread as I monitor potential pdoc issues in downstream dependencies via GitHub search.

I spent about an hour or two walking through this rabbit hole - this is a super nice experience, but pdoc doesn't support output to markdown.

I have no experience with docusaurus, but FWIW many Markdown-based frameworks will happily accept a blob of HTML in Markdown files. pdoc does that quite happily, all CSS is specifically scoped to not affect the rest of the page. You can find an example for mkdocs at https://github.com/mitmproxy/pdoc/tree/main/examples/mkdocs. We follow the same approach for mitmproxy (demo).

We also have some users that use a custom template to emit Markdown (https://github.com/codeghetti/seagulls-py/tree/main/.pdoc/templates), but ultimately it doesn't end up looking as nicely.

@pringshia
Copy link
Contributor

Thanks for the pointers @mhils - glad to know of the different approaches here. @JackUrb - I'm happy to revisit these approaches in a future update. As of now, I think this works for the 1.0 release, especially with the _target link fix.

@JackUrb
Copy link
Contributor Author

JackUrb commented Mar 14, 2022

I did a quick attempt at the implementation that mitmproxy uses, but found that Docusaurus isn't quite happy to have embedded HTML as it treats markdown files as React. In any case, we were able to find an implementation that works serving the static HTML on its own 'sub-site' of our docs. We can revisit the full customization choice that seagulls-py chose in the future if it turns out there's a strong need to colocate the API docs within the Docusaurus react tree (perhaps for automatic linking or something) in the future.

@JackUrb JackUrb merged commit 7f18e40 into dev-1.0 Mar 14, 2022
@JackUrb JackUrb deleted the missing-docs branch March 14, 2022 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants