search

qiboteam / qibotn

The tensor-network translation module for Qibo.
https://qibo.science
Apache License 2.0
3 stars 1 forks source link

Adding docs #35

Closed Vinitha-balachandran closed 4 months ago

Vinitha-balachandran commented 4 months ago

To add doc files

alecandido commented 4 months ago

TODO: point this PR to the branch of #24 (I can't do immediately, because there is no new commit wrt that one)

Motivation: having a meaningful diff (i.e. a meaningful view in the Files changed tab)

Vinitha-balachandran commented 4 months ago

TODO: point this PR to the branch of #24 (I can't do immediately, because there is no new commit wrt that one)

Motivation: having a meaningful diff (i.e. a meaningful view in the Files changed tab)

I have rebased PR to qibotn_integration branch.

liweintu commented 4 months ago

Heads-up: This PR is branched out from PR #24, that's why it has all the historical commits from branch qibotn_integration. We need to be careful about the two PR branches being merged into main.

alecandido commented 4 months ago

Heads-up: This PR is branched out from PR #24, that's why it has all the historical commits from branch qibotn_integration. We need to be careful about the two PR branches being merged into main.

I noticed, and @Vinitha-balachandran changed the base branch.

The worst it can happen now is that this will be merged in #24, increasing the diff. But it would not be a big deal anyhow, since this is mostly contained in a new folder.

alecandido commented 4 months ago

@liweintu and @Vinitha-balachandran is this completed?

I already had a look to the changes, and I have nothing major to amend.

The only piece that is clearly missing are the extracted docstrings. You could follow the Qibolab example: https://github.com/qiboteam/qibocal/blob/06457b828f0f8e4c1878266ea48924e6503b15a4/doc/source/conf.py#L126-L131 https://github.com/qiboteam/qibocal/blob/06457b828f0f8e4c1878266ea48924e6503b15a4/doc/source/index.rst?plain=1#L52-L56 (even though I'd gitignore the resulting folder, despite Qibocal is not doing it... but maybe it will!)

Other than that, I see that the PR is still draft, so I assume it to be work-in-progress. Is it correct?

liweintu commented 4 months ago

I noticed, and @Vinitha-balachandran changed the base branch.

The worst it can happen now is that this will be merged in #24, increasing the diff. But it would not be a big deal anyhow, since this is mostly contained in a new folder.

Indeed. Now that PR #35 is planned to be merged into the branch of PR #24 , we'll do PR #35 first. Then, followed by PR #24 . This should pick up all the commits from both branches.

alecandido commented 4 months ago

Indeed. Now that PR #35 is planned to be merged into the branch of PR #24 , we'll do PR #35 first. Then, followed by PR #24 . This should pick up all the commits from both branches.

That's acceptable, but if possible, I'd prioritize #24 first.

The rationale is that this is an addition, so it is not strictly needed for #24, and having #24 in master would also improve the testing for the PR in qibo itself.

liweintu commented 4 months ago

That's acceptable, but if possible, I'd prioritize #24 first.

The rationale is that this is an addition, so it is not strictly needed for #24, and having #24 in master would also improve the testing for the PR in qibo itself.

Yup, fair enough. We'll go through the remaining points in #24 and finish it up first.

Vinitha-balachandran commented 4 months ago

@liweintu and @Vinitha-balachandran is this completed?

I already had a look to the changes, and I have nothing major to amend.

The only piece that is clearly missing are the extracted docstrings. You could follow the Qibolab example: https://github.com/qiboteam/qibocal/blob/06457b828f0f8e4c1878266ea48924e6503b15a4/doc/source/conf.py#L126-L131 https://github.com/qiboteam/qibocal/blob/06457b828f0f8e4c1878266ea48924e6503b15a4/doc/source/index.rst?plain=1#L52-L56 (even though I'd gitignore the resulting folder, despite Qibocal is not doing it... but maybe it will!)

Other than that, I see that the PR is still draft, so I assume it to be work-in-progress. Is it correct?

I have included the missing part.

liweintu commented 4 months ago

Hi @alecandido , I see all comments are generally addressed, do you think it's good to merge this RP?

liweintu commented 4 months ago

That's acceptable, but if possible, I'd prioritize #24 first.

The rationale is that this is an addition, so it is not strictly needed for #24, and having #24 in master would also improve the testing for the PR in qibo itself.

Now that PR #24 has been merged into main, I changed the target branch to main as per the exact rationale above.

alecandido commented 4 months ago

The lock file was outdated. Every time you change dependencies in pyproject.toml (even groups and extras), please run poetry lock.

alecandido commented 4 months ago

Now that PR #24 has been merged into main, I changed the target branch to main as per the exact rationale above.

@scarrazza could you set the repo to automatically delete merged branches, and reroute existing PRs?

alecandido commented 4 months ago

Missing unused dependency

❯ make html
Running Sphinx v5.3.0

Extension error:
Could not import extension nbsphinx (exception: No module named 'nbsphinx')
make: *** [Makefile:20: html] Error 2

Please, try to install from scratch with poetry install --with docs, and then run cd docs; make html, to test the generation is happening correctly.

alecandido commented 4 months ago 
.../qibotn/docs/source/index.rst:46: ERROR: Error in "toctree" directive:
invalid option block.

.. toctree::
   :maxdepth: 2
   :caption: Introduction
   Installation
   Quickstart
.../qibotn/docs/source/index.rst:44: ERROR: Document or section may notbegin with a transition.
liweintu commented 4 months ago 
.../qibotn/docs/source/index.rst:46: ERROR: Error in "toctree" directive:
invalid option block.

.. toctree::
   :maxdepth: 2
   :caption: Introduction
   Installation
   Quickstart
.../qibotn/docs/source/index.rst:44: ERROR: Document or section may notbegin with a transition.

After trying poetry install --with docs, I re-produced the error, will follow some reference from qibo or qibocal to fix it.

liweintu commented 4 months ago

Missing unused dependency

❯ make html
Running Sphinx v5.3.0

Extension error:
Could not import extension nbsphinx (exception: No module named 'nbsphinx')
make: *** [Makefile:20: html] Error 2

Please, try to install from scratch with poetry install --with docs, and then run cd docs; make html, to test the generation is happening correctly.

After re-organising the files into the subfolders as per Qibo document convention, make html can work.

However, I encountered a hanging issue of poetry install in some GPU Ubuntu machine. In some other machine (e.g. MacBook), poetry install works fine. @Tankya2 also experienced this. It seems to be a known issue as per here: python-poetry/poetry#3365. Did you ever encounter this hanging issue? @alecandido

scarrazza commented 4 months ago

@scarrazza could you set the repo to automatically delete merged branches, and reroute existing PRs?

Done.

alecandido commented 4 months ago

It seems to be a known issue as per here: python-poetry/poetry#3365. Did you ever encounter this hanging issue? @alecandido

Yes, it was a common problem around Poetry v1.1.2 and similar. By now (Poetry v1.8 released few days ago, I'm also using v1.7.1), I've never encountered any longer.

If the version of Poetry you're using, I'd suggest upgrading it (with the various methods for installation listed in the docs, e.g. pipx, but it's just an example), and deleting the cache (poetry cache clear PyPI, or poetry cache clear <id>, where you could find <id> running poetry cache list).

@liweintu