-
Notifications
You must be signed in to change notification settings - Fork 35
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
Create "Quick starting guide to running MESSAGEix-GLOBIOM" for occasional users #157
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #157 +/- ##
=====================================
Coverage 76.7% 76.7%
=====================================
Files 112 112
Lines 7154 7154
=====================================
Hits 5494 5494
Misses 1660 1660 |
Curious as to why the tests fail to read the snapshot. Is there some kind of rate limit with Zenodo that we are hitting? |
Overall this seems like a good start! Things that jump out at a quick skim:
Any test (or CI step) that makes a network query is vulnerable to transient connection issues: DNS lookups, the server responding, completing the query, etc. Some of these are not about our code but about the availability and steadiness of Zenodo's services. When we try this enough times, a low failure probability can yield frequent single failures. |
doc/install.rst
Outdated
----------- | ||
|
||
Use this option if you intend to make changes to the source code. | ||
We value your contributions via pull requests to `the main repository <https://github.com/iiasa/message-ix-models>`_. Please consider `contributing to development <https://docs.messageix.org/en/latest/contributing.html>`_ your changes. |
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 is a direct URL link:
`contributing to development <https://docs.messageix.org/en/latest/contributing.html>`_
Wherever possible we use intersphinx links, which are more reliable:
:doc:`message-ix:contributing`
- The prefixes before the
:
can be viewed in conf.py:Lines 134 to 150 in 583d3e9
intersphinx_mapping = { "click": ("https://click.palletsprojects.com/en/8.1.x/", None), "genno": ("https://genno.readthedocs.io/en/stable", None), "ixmp": ("https://docs.messageix.org/projects/ixmp/en/latest/", None), "message-ix": ("https://docs.messageix.org/en/latest/", None), "m-data": ( f"https://{_token}:@docs.messageix.org/projects/models-internal/en/latest/", # Use a local copy of objects.inv, if the user has one (local_inv("message_data"), None), ), "pandas": ("https://pandas.pydata.org/pandas-docs/stable/", None), "pint": ("https://pint.readthedocs.io/en/stable/", None), "pooch": ("https://www.fatiando.org/pooch/latest/", None), "pytest": ("https://docs.pytest.org/en/stable/", None), "python": ("https://docs.python.org/3/", None), "sdmx": ("https://sdmx1.readthedocs.io/en/stable/", None), } - The docs show how to retrieve and inspect the list of :doc:, :ref: etc. targets available from a given other project.
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.
The command to retrieve the targets doesn't readily work for me. If I pass a directory to it, it gives me an IsADirectoryError
; if I pass in a single file, I receive varying ValueError
s because the inventory header is invalid; and the same is true if I pass in a URL to a page of our docs.
I guess I'll look up targets manually.
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.
Notice that in the example in the Sphinx docs, …/objects.inv
is appended. This happens automatically during sphinx-build
, but not for the helper command.
So for instance: python -m sphinx.ext.intersphinx https://pint.readthedocs.io/en/stable/objects.inv
, where "https://pint.readthedocs.io/en/stable/" is the base URL appearing in conf.py. I usually pipe the result to less
to be able to page up and down.
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, it needs to be the objects.inv
file that I can find in e.g. ixmp/doc/_build/
.
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 I understand correctly, intersphinx tries to resolve an undefined reference like :doc:'installation'
by going through all other linked sources, so I tried following the official guide and linked external reference in this way: :external+message-ix:doc:'installation'
.
This merge to #287 is super helpful. Thank you. |
The docs build might not work yet because I already included some docs labels from message_ix that are only yet introduced with iiasa/message_ix#805. Apart from that, I rewrote parts of the new docs to avoid duplicate information, but there's actually not a lot of copy-paste in here to begin with, so it's still the same in large parts. |
We already do that: https://docs.messageix.org/en/stable/install.html |
Also, there's an abundance of unrecognized/unresolvable references in other documents and it would likely improve our docs a lot if we fixed them, but this is a topic for another PR. |
@jkikstra This has been a long time ago for you, but at today's MESSAGE meeting we concluded that it would probably still be good for you to take a look at the final product and tell us if you like it. |
* Expand install instructions
* Prefer intersphinx links over weblinks * Rewrite to avoid duplicate information
7416c79
to
0f5612e
Compare
- Add and use :gh-user:`…` Sphinx role. - Start new sentences on new lines in ReST. - Use :doc:`text <other-project:target>` instead of :external+other-project:doc:`text <target>` - Use :file:`…` for file and directory paths. - Use :mod:`...` for other packages and modules. - Expand description of CLI and link to ixmp docs. - Promote use of .snapshot.load(), before explaining how to manually imitate its behaviour.
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 added a commit to the branch with some copyedits and additions.
LGTM once checks pass!
Thanks :) |
This PR supersedes https://github.com/iiasa/message_data/pull/312 and thus fixes https://github.com/iiasa/message_data/issues/287 once we merge it. The main reason for migrating this PR is that we now have a snapshot of a global baseline model available at Zenodo, so there might be demand for having these instructions be public, too.
Thanks a lot to @jkikstra for writing up the original issue and to @LauWien for starting the original PR.
After going through the original material, I think almost everything should be included in the new docs except for some Common Issue that @jkikstra reported:
The first one shouldn't happen if users follow the install instructions closely; the second one seems rather general, so should maybe make it into message_ix if we want to record it, but isn't directly related to install issues; the third one might also need to go to message_ix and is also only related to running the code without errors.
How to review
@gidden, since you were interested in merging https://github.com/iiasa/message_data/pull/312, could you please check that no information in there is incorrect?
@yiyi1991, @daymontas1: since you have started recently, maybe you could take a look at the new docs and see if they would have been helpful for you. If you notice anything that seems to be missing, please leave a comment :)
FYI @khaeru, I might enable building the docs on RTD for this PR to make it easier to check the new ones.
PR checklist
[ ] Add or expand tests; coverage checks both ✅Only updating docs.