-
Notifications
You must be signed in to change notification settings - Fork 150
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
[Docs] Add docs with mkdocs
#202
[Docs] Add docs with mkdocs
#202
Conversation
1c206ec
to
260a824
Compare
5b16a7a
to
b96b18b
Compare
I made a few additional changes:
|
ml_metadata/__init__.py
Outdated
__all__ = [ | ||
"downgrade_schema", | ||
"ListOptions", | ||
"MetadataStore", | ||
"OrderByField", | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't in scope for this PR, but the try/except block above doesn't work. In the event of an ImportError
, this module won't be able to export these symbols. I can't really tell what is intended by the try/except block swallowing any import errors, but maybe this is something we can clean up in a future PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to have been present since before we started working on the repo.
I agree with you that we can possibly revisit this in a future PR
del metadata_store_service_pb2 | ||
del metadata_store_service_pb2_grpc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're going to remove the del metadata_store_pb2
line, can you also remove these other two?
Just a note of context for others working on this: I'm assuming this is to "un-import" the imported protobuf modules...but I'm not clear on why this is needed. If the goal is to re-export certain symbols, from <module> import <symbol>
is sufficient. del <module>
doesn't "un-import" the module - it persists elsewhere. See https://mail.python.org/pipermail/tutor/2006-August/048596.html for details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed del metadata_store_pb2
because as it is currently written, it is not imported directly anymore. It just has member objects imported. I left the other two del
statements because they are not used to import member objects. My preference is to leave them as they are and revisit them in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor changes/questions. Once those are done, if you've checked that this deploys and looks good, then let's ship it! 🚢
.github/workflows/cd-docs.yml
Outdated
branches: | ||
- master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peytondmurray I think we should allow this to run for push on any branch so developers can preview the deployment from their forks. If we make the reasonable assumption that nobody is pushing directly to upstream/master, then this will not affect the main docs deployment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now we have no way of deploying builds of individual branches - for now it is up to the users to build the docs and examine their changes locally, and for reviewers to pay due diligence in asking PR authors to provide screenshots/description of major changes to the docs.
Nobody will be pushing to master
, but we still want this enabled so that when github itself makes a merge commit and pushes it to master the docs will be rebuilt. As it stands every PR will force-push to gh-pages
.
6bb2e31
to
a4d2b20
Compare
a4d2b20
to
ca825a5
Compare
@peytondmurray fixed merge conflict and marked as ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this 🚀
Hi, could you please resolve the conflicts in this branch? |
Add pre-commit and associated CI
Links do not work because API for `mlmd.proto` is not showing up yet. Links will work when this is fixed
303f72f
to
4525700
Compare
Done CC: @peytondmurray |
mkdocs
mkdocs
@XinranTang Is there anything else I can do to push this through? It would be nice to get this merged. |
@XinranTang Thanks for merging this. Can you please change the publishing branch to |
Thanks @XinranTang! I somehow overlooked adding some of the documentation content to the branch before opening the PR. I have added it now in PR #208. Can you please review and merge it? Sorry for the confusion. |
Google Would like to migrate the documentation for
mlmd
away from their internal systems, so this PR implements that migration.Changes in this PR:
.gitignore
to the projectmkdocs
to the dependencies and add a configuration for it__all__
variables so modules and their members are recognized bymkdocs
TODO:
proto
submodule