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

[Docs] Update PyTorch tutorial #2063

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

adarshan-intel
Copy link
Contributor

@adarshan-intel adarshan-intel commented Nov 18, 2024

Description of the Changes

This Pull Request (PR) addresses documentation updates in the Gramine project, influenced by changes made in gramineproject/examples#109

How to Test This PR?

This PR should be merged after the completion and merging of gramineproject/examples#109


This change is Reviewable

@adarshan-intel adarshan-intel force-pushed the adarsh/pytorch_docs_24_04 branch from dd13367 to afe6d88 Compare November 18, 2024 09:53
Copy link
Contributor

@efu39 efu39 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: )

@adarshan-intel adarshan-intel force-pushed the adarsh/pytorch_docs_24_04 branch from afe6d88 to 208c8ac Compare November 27, 2024 06:12
@adarshan-intel
Copy link
Contributor Author

adarshan-intel commented Nov 28, 2024

@kailun-qin Please review

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @adarshan-intel)


-- commits line 2 at r1:
-> [Docs] Update PyTorch tutorial

Code quote:

[Docs] Update pytorch docs to add support for Ubuntu 24.04

Documentation/tutorials/pytorch/index.rst line 95 at r1 (raw file):

  the compute platform).

- Gramine v1.8, with DCAP support. DCAP software infrastructure must also be

We remove the notes above but we still pin to a specific Gramine version?

Code quote:

Gramine v1.8

Documentation/tutorials/pytorch/index.rst line 102 at r1 (raw file):

.. warning::

   After running Ubuntu 24.04, it is highly recommended to use a virtual environment to manage

Is this recommended or required for Ubuntu 24.04?

Code quote:

After running Ubuntu 24.04, it is highly recommended to use a virtual environment

Documentation/tutorials/pytorch/index.rst line 124 at r1 (raw file):

Go to the directory with Gramine's PyTorch example::

   git clone --depth 1 --branch v1.8 https://github.com/gramineproject/examples.git

ditto (pin to specific version). And this branch does not contain your updates to the PyTorch example -- meaning that this won't actually work for Ubuntu 24.04?

Code quote:

--branch v1.8

Documentation/tutorials/pytorch/index.rst line 377 at r1 (raw file):

repository), so let's build the secret provisioning server::

   git clone --depth 1 --branch v1.8 https://github.com/gramineproject/gramine.git

ditto

Code quote:

v1.8

Copy link
Contributor Author

@adarshan-intel adarshan-intel left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @kailun-qin)


-- commits line 2 at r1:

Previously, kailun-qin (Kailun Qin) wrote…

-> [Docs] Update PyTorch tutorial

Done.


Documentation/tutorials/pytorch/index.rst line 95 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

We remove the notes above but we still pin to a specific Gramine version?

Done.
Removing pinning of versions


Documentation/tutorials/pytorch/index.rst line 102 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Is this recommended or required for Ubuntu 24.04?

It is a must case, so making it required


Documentation/tutorials/pytorch/index.rst line 124 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

ditto (pin to specific version). And this branch does not contain your updates to the PyTorch example -- meaning that this won't actually work for Ubuntu 24.04?

Done.
Removing pinning of versions


Documentation/tutorials/pytorch/index.rst line 377 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

ditto

Done.

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @adarshan-intel)


Documentation/tutorials/pytorch/index.rst line 99 at r2 (raw file):

Please refer to the `Gramine Attestation Documentation
<https://gramine.readthedocs.io/en/latest/attestation.html>`__ for more details.

Pls align and wrap.

Code quote:

- Gramine with DCAP support. DCAP software infrastructure must also be
   installed.

Please refer to the `Gramine Attestation Documentation
<https://gramine.readthedocs.io/en/latest/attestation.html>`__ for more details.

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @adarshan-intel)


-- commits line 2 at r1:

Previously, adarshan-intel (Adarsh Anand) wrote…

Done.

Not done?


Documentation/tutorials/pytorch/index.rst line 124 at r1 (raw file):

Previously, adarshan-intel (Adarsh Anand) wrote…

Done.
Removing pinning of versions

But why remove --depth 1?


Documentation/tutorials/pytorch/index.rst line 377 at r1 (raw file):

Previously, adarshan-intel (Adarsh Anand) wrote…

Done.

But why remove --depth 1?

Copy link
Member

@woju woju left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @adarshan-intel and @kailun-qin)


Documentation/tutorials/pytorch/index.rst line 377 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

But why remove --depth 1?

This is wrong: when you clone something from git without pinning to a tag, you might get newer version than packages support. Recently we had this problem with entrypoint: the option was removed from manifests in examples, but the released (packaged) version still required it, so if someone just cloned examples, they didn't work. So no, you need to keep --branch.

If you want this to be moved to last stable release, you can use |stable-checkout| like this (note parsed-literal instead of ::-block):

.. parsed-literal::

   git clone --depth 1 |stable-checkout| \https://github.com/gramineproject/examples.git

cf.:

.. parsed-literal::
git clone --depth 1 |stable-checkout| \https://github.com/gramineproject/gramine.git

I'm not sure about examples.git, they might be OK to just clone, so I didn't block the other discussion.


Documentation/tutorials/pytorch/index.rst line 88 at r2 (raw file):

-------------

- A Linux-based operating system (e.g., Ubuntu 20.04 or later)

Gramine supports 22.04 or later, please update. I know that maybe this howto works with 20.04, but we don't want people to file issues for 20.04 anymore.

Suggestion:

Ubuntu 22.04

Documentation/tutorials/pytorch/index.rst line 103 at r2 (raw file):

.. warning::

   After running Ubuntu 24.04, it is required to use a virtual environment to manage

"After running Ubuntu 24.04" sounds weird, needs to be rephrased. Something like "If you're on a recent distro that implements PEP 668 (e.g. Debian 12, Ubuntu 24.04), it is required ..."?

@adarshan-intel adarshan-intel force-pushed the adarsh/pytorch_docs_24_04 branch from aca2ee7 to 326224c Compare December 5, 2024 04:08
Copy link
Contributor Author

@adarshan-intel adarshan-intel left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @woju)


-- commits line 2 at r1:

Previously, kailun-qin (Kailun Qin) wrote…

Not done?

Done.


Documentation/tutorials/pytorch/index.rst line 124 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

But why remove --depth 1?

Done.


Documentation/tutorials/pytorch/index.rst line 377 at r1 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

This is wrong: when you clone something from git without pinning to a tag, you might get newer version than packages support. Recently we had this problem with entrypoint: the option was removed from manifests in examples, but the released (packaged) version still required it, so if someone just cloned examples, they didn't work. So no, you need to keep --branch.

If you want this to be moved to last stable release, you can use |stable-checkout| like this (note parsed-literal instead of ::-block):

.. parsed-literal::

   git clone --depth 1 |stable-checkout| \https://github.com/gramineproject/examples.git

cf.:

.. parsed-literal::
git clone --depth 1 |stable-checkout| \https://github.com/gramineproject/gramine.git

I'm not sure about examples.git, they might be OK to just clone, so I didn't block the other discussion.

I believe cloning examples repo would be just fine, as of not just keeping to latest commit with depth 1 will change upon request


Documentation/tutorials/pytorch/index.rst line 88 at r2 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

Gramine supports 22.04 or later, please update. I know that maybe this howto works with 20.04, but we don't want people to file issues for 20.04 anymore.

Done.


Documentation/tutorials/pytorch/index.rst line 99 at r2 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Pls align and wrap.

Done.


Documentation/tutorials/pytorch/index.rst line 103 at r2 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

"After running Ubuntu 24.04" sounds weird, needs to be rephrased. Something like "If you're on a recent distro that implements PEP 668 (e.g. Debian 12, Ubuntu 24.04), it is required ..."?

Done.

Copy link
Member

@woju woju left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin)


-- commits line 2 at r1:

Previously, adarshan-intel (Adarsh Anand) wrote…

Done.

No, you didn't. You need to either rework the commits itself (git commit --amend and/or git rebase -i --autosquash $(git merge-base HEAD origin/master) — be careful not to move branch base, because it messes reviewer's workflow), or you need to post amend! commit (git commit --fixup=amend:...) for this review discussion to be "Done".


Documentation/tutorials/pytorch/index.rst line 377 at r1 (raw file):

Previously, adarshan-intel (Adarsh Anand) wrote…

I believe cloning examples repo would be just fine, as of not just keeping to latest commit with depth 1 will change upon request

Yes, but here you clone gramine.git itself. As I explained, you need to specify --branch, either explicitly, or through |stable-checkout|.

@adarshan-intel
Copy link
Contributor Author

Documentation/tutorials/pytorch/index.rst line 377 at r1 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

Yes, but here you clone gramine.git itself. As I explained, you need to specify --branch, either explicitly, or through |stable-checkout|.

Earlier, I was confused why |stable-checkout| was not working. Then I wrapped it with backticks (``), and it worked. I believe this is what you were suggesting?

@adarshan-intel adarshan-intel changed the title [Docs] Update pytorch docs to add support for Ubuntu 24.04 [Docs] Update PyTorch tutorial Dec 6, 2024
@adarshan-intel adarshan-intel force-pushed the adarsh/pytorch_docs_24_04 branch from 1565246 to 6afe727 Compare December 6, 2024 05:44
Copy link
Contributor Author

@adarshan-intel adarshan-intel left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @kailun-qin and @woju)


-- commits line 2 at r1:

Previously, woju (Wojtek Porczyk) wrote…

No, you didn't. You need to either rework the commits itself (git commit --amend and/or git rebase -i --autosquash $(git merge-base HEAD origin/master) — be careful not to move branch base, because it messes reviewer's workflow), or you need to post amend! commit (git commit --fixup=amend:...) for this review discussion to be "Done".

I have done interactive rebase.
Can you check if it is done now?

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @woju)


Documentation/tutorials/pytorch/index.rst line 377 at r1 (raw file):

Then I wrapped it with backticks (``), and it worked

What you mean by working?

I think what woju was suggesting is to use the parsed-literal reST directive for that "git clone ..." block of text, same as here:

.. parsed-literal::
git clone --depth 1 |stable-checkout| \https://github.com/gramineproject/gramine.git
.

@adarshan-intel
Copy link
Contributor Author

Documentation/tutorials/pytorch/index.rst line 377 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Then I wrapped it with backticks (``), and it worked

What you mean by working?

I think what woju was suggesting is to use the parsed-literal reST directive for that "git clone ..." block of text, same as here:

.. parsed-literal::
git clone --depth 1 |stable-checkout| \https://github.com/gramineproject/gramine.git
.

Something like this?

@adarshan-intel
Copy link
Contributor Author

Documentation/tutorials/pytorch/index.rst line 377 at r1 (raw file):

Previously, adarshan-intel (Adarsh Anand) wrote…

Something like this?

image.png
But when I run make html, the documentation doesn’t display correctly

Copy link
Member

@woju woju left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @adarshan-intel and @kailun-qin)


-- commits line 2 at r1:

Previously, adarshan-intel (Adarsh Anand) wrote…

I have done interactive rebase.
Can you check if it is done now?

Yes, you did rebase, but you didn't change the title of the commit as @kailun-qin asked (and then proceeded to add a couple of extra fixup! commits). Still not done.


Documentation/tutorials/pytorch/index.rst line 377 at r1 (raw file):

Previously, adarshan-intel (Adarsh Anand) wrote…

image.png
But when I run make html, the documentation doesn’t display correctly

Yes, that's expected: it's hidden until you tag the version and add READTHEDOCS=1 READTHEDOCS_VERSION=stable to make html. Unless you do that, it will be hidden, as it should.

There's a catch though, it currently fails without this PR: #2073.

make clean
git tag v123456-TEST
make html READTHEDOCS=1 READTHEDOCS_VERSION=stable
git tag -d v123456-TEST
firefox _build/html/index.html

So this particular section LGTM. I've tested it provisionally rebased on top of #2073 and it works fine (with double-colon fixed to single colon):

image.png


Documentation/tutorials/pytorch/index.rst line 376 at r8 (raw file):

We will use the reference implementation of the Secret Provisioning server found
under ``CI-Examples/ra-tls-secret-prov`` directory (in the core Gramine
repository), so let's build the secret provisioning server::

This double colon you need to fix to single colon for the block to render properly (see screenshot in next discussion).

Suggestion:

server:

@adarshan-intel
Copy link
Contributor Author

-- commits line 2 at r1:

Yes, you did rebase, but you didn't change the title of the commit as @kailun-qin asked (and then proceeded to add a couple of extra fixup! commits). Still not done.

image.png
In my commit history I can see the name, [Docs] Update PyTorch tutorial, this is the name which is expected right?
Or am I missing something>

@adarshan-intel
Copy link
Contributor Author

Documentation/tutorials/pytorch/index.rst line 376 at r8 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

This double colon you need to fix to single colon for the block to render properly (see screenshot in next discussion).

Done

Copy link
Contributor Author

@adarshan-intel adarshan-intel left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @woju)


Documentation/tutorials/pytorch/index.rst line 377 at r1 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

Yes, that's expected: it's hidden until you tag the version and add READTHEDOCS=1 READTHEDOCS_VERSION=stable to make html. Unless you do that, it will be hidden, as it should.

There's a catch though, it currently fails without this PR: #2073.

make clean
git tag v123456-TEST
make html READTHEDOCS=1 READTHEDOCS_VERSION=stable
git tag -d v123456-TEST
firefox _build/html/index.html

So this particular section LGTM. I've tested it provisionally rebased on top of #2073 and it works fine (with double-colon fixed to single colon):

image.png

Done.

Copy link
Member

@woju woju left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @adarshan-intel and @kailun-qin)


-- commits line 2 at r1:

Previously, adarshan-intel (Adarsh Anand) wrote…

Yes, you did rebase, but you didn't change the title of the commit as @kailun-qin asked (and then proceeded to add a couple of extra fixup! commits). Still not done.

image.png
In my commit history I can see the name, [Docs] Update PyTorch tutorial, this is the name which is expected right?
Or am I missing something>

Ah, ok. I misread, sorry for the confusion.


Documentation/tutorials/pytorch/index.rst line 376 at r8 (raw file):

Previously, adarshan-intel (Adarsh Anand) wrote…

Done

Not done, wrong server::.


Documentation/tutorials/pytorch/index.rst line 446 at r9 (raw file):

and use them.

Now we can launch the secret provisioning server:

This was OK, please revert.

Suggestion:

server::

Copy link
Contributor Author

@adarshan-intel adarshan-intel left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @woju)


Documentation/tutorials/pytorch/index.rst line 376 at r8 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

Not done, wrong server::.

Done.
Good catch, I made changes at other place.


Documentation/tutorials/pytorch/index.rst line 446 at r9 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

This was OK, please revert.

Done.

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @woju)

Copy link
Contributor Author

@adarshan-intel adarshan-intel left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @woju)


Documentation/tutorials/pytorch/index.rst line 376 at r8 (raw file):

Previously, adarshan-intel (Adarsh Anand) wrote…

Done.
Good catch, I made changes at other place.

Done


Documentation/tutorials/pytorch/index.rst line 446 at r9 (raw file):

Previously, adarshan-intel (Adarsh Anand) wrote…

Done.

Done

Copy link
Contributor Author

@adarshan-intel adarshan-intel left a comment

Choose a reason for hiding this comment

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

@woju Have updated the server: typo at both places, any more changes TODO ?

Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @woju)

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.

4 participants