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 interactive environments documentation #1737

Merged
merged 3 commits into from
Dec 4, 2020

Conversation

lorenzo-cavazzi
Copy link
Member

Update the Interactive Environments documentation pages, including the feature to pin images (that requires this to the merged SwissDataScienceCenter/renku-ui#1109).

fix #1647

@lorenzo-cavazzi lorenzo-cavazzi added this to the sprint-2020-11-13 milestone Nov 25, 2020
@lorenzo-cavazzi lorenzo-cavazzi requested a review from a team November 25, 2020 17:02
Copy link
Member

@rokroskar rokroskar left a comment

Choose a reason for hiding this comment

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

This is great @lorenzo-cavazzi thanks!! Just a few minor comments. My future self (hunting for renku.ini options all over the docs) thanks you in advance 🙇


* auto-saving of work back to RenkuLab, so you can recover in the event of a
crash
* Auto-saving of work back to RenkuLab, so you can recover in the event of a crash.
Copy link
Member

Choose a reason for hiding this comment

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

hmm this is not technically correct. We do not prevent loss of work if the infrastructure actually crashes.... I know it's not added in this PR but just a point to consider

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. We should mention a more generic "environment shutdown". We can also mention that environments are shut down automatically after 24h of inactivity


(This is defined in the project's ``.gitlab-ci.yml`` file.)
This is defined in the project's :ref:`.gitlab-ci.yml file <gitlab_ci_yml>`. If the project
Copy link
Member

Choose a reason for hiding this comment

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

"this" could be taken to refer to "when any of the following happens" - which is not technically incorrect but maybe it's easier to say that the pipeline is defined:

Suggested change
This is defined in the project's :ref:`.gitlab-ci.yml file <gitlab_ci_yml>`. If the project
The pipeline is defined in the project's :ref:`.gitlab-ci.yml file <gitlab_ci_yml>`. If the project


(This is defined in the project's ``.gitlab-ci.yml`` file.)
This is defined in the project's :ref:`.gitlab-ci.yml file <gitlab_ci_yml>`. If the project
references a pinned image, the UI will not check for the image availability - that is
Copy link
Member

Choose a reason for hiding this comment

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

small wording change suggestion - and maybe we should link to the relevant section describing this option here?

Suggested change
references a pinned image, the UI will not check for the image availability - that is
references a specific image to use for all environments, the UI will not check for the image availability - that is

the default ``Dockerfile`` provided in the template, the first line is a
``RENKU_BASE_IMAGE`` argument used to feed the following ``FROM`` instruction.
It specifies a
`versioned base docker image <https://github.com/SwissDataScienceCenter/renku-jupyter>`_.
Copy link
Member

Choose a reason for hiding this comment

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

oops this link is wrong

Suggested change
`versioned base docker image <https://github.com/SwissDataScienceCenter/renku-jupyter>`_.
`versioned base docker image <https://github.com/SwissDataScienceCenter/renkulab-docker>`_.

Manually modifying the ``renku.ini`` file is not recommended.
You can use the
`renku config command <https://renku-python.readthedocs.io/en/latest/commands.html#module-renku.cli.config>`_
form an interactive environment.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
form an interactive environment.
from an interactive environment, for example:


.. note::

Manually modifying the ``renku.ini`` file is not recommended.
Copy link
Member

Choose a reason for hiding this comment

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

Oh really? That's the only way I ever do it 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

ahah it may work for you, but I would say it's safer to use the renku command line to avoid messing up the config file 😁

Copy link
Member

Choose a reason for hiding this comment

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

I always just assume the UI is buggy and move on 😆


renku config set interactive.default_url "/tree"

We are working on adding a user friendly solution to set default options on
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
We are working on adding a user friendly solution to set default options on
We are working on adding a user-friendly solution to set default options on

Copy link
Member Author

Choose a reason for hiding this comment

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

👍
I'll fix it also on the other locations where we use the space

@@ -164,6 +174,58 @@ these base ``Dockerfile`` s and add the ``renku``, ``git``, and ``jupyter``
parts to another base image that you might have.


Renku project configurations
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can move this section before the Docker section? I imagine people might want to change this more often than they would want to dig into the Docker build itself.

@lorenzo-cavazzi
Copy link
Member Author

lorenzo-cavazzi commented Nov 30, 2020

Thanks for the review. All the points should be addressed.

rokroskar
rokroskar previously approved these changes Dec 1, 2020
Copy link
Contributor

@ciyer ciyer left a comment

Choose a reason for hiding this comment

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

Lots of good improvements here. I think we need to be a bit clearer about the pinned image feature.

language-specific dependency-management facilities (``requirements.txt``,
``install.R``, etc.) or installed in the ``Dockerfile``
``install.R``, etc.) or installed in the ``Dockerfile``.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add to this, I think.

* Any dependencies you specified via conda (``environment.yml``), using language-specific dependency-management facilities (``requirements.txt``, ``install.R``, etc.) or installed in the ``Dockerfile``. (An exception to this is if the project uses a pinned image. [See the section below](linked to pinned image section) for more information.)

Copy link
Member

Choose a reason for hiding this comment

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

@ciyer I thought we should try to avoid using the word "pinned" - I don't think it's really a term that most of our users understand immediately. But maybe it's more widely used than I think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@rokroskar I wasn't involved with those discussions, so I don't know what arguments were presented.

I just do not find "custom" very descriptive in this case. There image isn't any more or less customized than any other image used in an interactive environment -- what is unique in this case is that the image if pinned/fixed and may not reflect the contents of the project (Dockerfile, environment.yml, etc.).

In fact, the project that was created for testing was called "fixed-image" -- that's an indication to me that there might be a naming mismatch.

Copy link
Member

Choose a reason for hiding this comment

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

@ciyer there wasn't really a discussion, it was just a suggestion I made here - I'm fine with using "pinned" but I thought we could maybe come up with something a bit less technical. If there isn't a better term then pinned is also OK.

It's custom in the sense that it's not the one automatically provided by the system. It might be one of our standard images, one that was built by a previous CI run, or something that the user created completely on their own.

over a short period of time (e.g. you use it in a presentation or a lecture).

Even if it's common to start the environment with the default values, setting a default value
doesn't prevent a user from changing it.
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think a user can change the pinned image, or am I wrong?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is true in the current UI implementation, but I'm not sure if we want to change that or not.
The section is about all the options, but you are right, image is an exception. If you think it's better to specify it, I can add the text

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should describe the current UI behavior in the documentation. If that changes, then we should update the documentation.

@lorenzo-cavazzi
Copy link
Member Author

I addressed the 2 points from the last review, but I didn't use the term pinned image.
@rokroskar @ciyer if we want to use a specific term (fixed/pinned/custom/ image) it's better to settle on a single one and keep it coherent both here in the docs and in the UI.

@ciyer
Copy link
Contributor

ciyer commented Dec 2, 2020

I addressed the 2 points from the last review, but I didn't use the term pinned image.
@rokroskar @ciyer if we want to use a specific term (fixed/pinned/custom/ image) it's better to settle on a single one and keep it coherent both here in the docs and in the UI.

I have argued my point. In the end, it is @rokroskar 's decision.

@rokroskar
Copy link
Member

@lorenzo-cavazzi I think go with "pinned" and if people respond with bewilderment we can revise :)

Copy link
Contributor

@ciyer ciyer left a comment

Choose a reason for hiding this comment

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

👍

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.

Add documentation for pinning a registry image
3 participants