feat: content render and restart #236

tdstein · 2024-07-22T19:32:33Z

Add support for content.restart() and content.refresh().

integration/resources/bundles/example-flask-minimal/hello.py

github-actions · 2024-07-22T19:33:33Z

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines	Covered	Coverage	Threshold	Status
1226	1157	94%	0%	🟢

New Files

File	Coverage	Status
src/posit/connect/env.py	100%	🟢
src/posit/connect/variants.py	100%	🟢
TOTAL	100%	🟢

Modified Files

File	Coverage	Status
src/posit/connect/content.py	100%	🟢
TOTAL	100%	🟢

updated for commit: cb78d63 by action🐍

tdstein · 2024-07-22T19:34:18Z

pyproject.toml

@@ -25,6 +25,9 @@ dependencies = ["requests>=2.31.0,<3"]
 Source = "https://github.com/posit-dev/posit-sdk-py"
 Issues = "https://github.com/posit-dev/posit-sdk-py/issues"

+[tool.mypy]
+exclude = "integration/resources/*"


Ignore the resources folder with static type checking.

tdstein · 2024-07-22T19:34:30Z

pyproject.toml

@@ -34,6 +37,7 @@ version_file = "src/posit/_version.py"

 [tool.ruff]
 line-length = 79
+exclude = ["integration/resources/*"]


Ignore the resources folder when linting

src/posit/connect/content.py

tdstein · 2024-07-22T19:37:35Z

src/posit/connect/content.py

+    @property
+    def _variants(self) -> Variants:
+        return Variants(self.config, self.session, self.guid)


This is intentionally marked as private. We don't want customers to interact directly with the variants API since it isn't a v1 API.

We'll add a v1 version, but even so, you'll still have to fall back to the old APIs when you're on old (well, contemporary) Connect. This is fine for this PR, since I'm guessing you're not implementing all of the Variants features (I haven't read that far yet), but just wanted to put that out there for the future. It's been in production for years, people use it, so our notion of internal vs. public doesn't really matter.

src/posit/connect/env.py

tdstein · 2024-07-22T19:40:44Z

src/posit/connect/env.py

+        d: Dict[str, str] = {}
+        if other is not None:
+            if isinstance(other, MutableMapping):
+                d.update(other)
+            elif isinstance(other, Iterable) and not isinstance(
+                other, (str, bytes)
+            ):
+                try:
+                    d.update(other)
+                except (TypeError, ValueError):
+                    raise TypeError(
+                        f"update expected a {MutableMapping} or {Iterable}, got {type(other)}"
+                    )
+            else:
+                raise TypeError(
+                    f"update expected a {MutableMapping} or {Iterable}, got {type(other)}"
+                )
+
+        if kwargs:
+            d.update(kwargs)


This logic mimics the dict.update behavior.

tdstein · 2024-07-22T19:43:05Z

integration/tests/posit/connect/test_content.py

+    @pytest.mark.skipif(
+        CONNECT_VERSION <= version.parse("2023.01.1"),
+        reason="Python 3.12 not available",
+    )
+    def test_restart(self):
+        # create content
+        content = self.client.content.create(name="example-flask-minimal")
+        # create bundle
+        path = Path(
+            "../../../resources/bundles/example-flask-minimal/bundle.tar.gz"
+        )
+        path = (Path(__file__).parent / path).resolve()
+        bundle = content.bundles.create(str(path))
+        # deploy bundle
+        task = bundle.deploy()
+        task.wait_for()
+        # restart content
+        content.restart()
+        # delete content
+        content.delete()
+
+    @pytest.mark.skipif(
+        CONNECT_VERSION <= version.parse("2023.01.1"),
+        reason="Quarto not available",
+    )
+    def test_refresh(self):
+        # create content
+        content = self.client.content.create(name="example-quarto-minimal")
+        # create bundle
+        path = Path(
+            "../../../resources/bundles/example-quarto-minimal/bundle.tar.gz"
+        )
+        path = (Path(__file__).parent / path).resolve()
+        bundle = content.bundles.create(str(path))
+        # deploy bundle
+        task = bundle.deploy()
+        task.wait_for()
+        # refresh content
+        task = content.refresh()
+        if task:
+            task.wait_for()
+
+        # delete content
+        content.delete()


I'm still working on adding support for the skipped versions of Connect, but I'll make a separate change. Theres a bit involved to make it work, so it deserves its own pull request.

tdstein · 2024-07-22T19:47:28Z

src/posit/connect/content.py

+    def _re_whatever(self) -> Task | None:
+        """Submit a re-whatever request (i.e., restart, refresh, etc).
+
+        A re-whatever is a catch-all term for restarting, refreshing, or re-whatever-ing the content requires to bounce it to a new state.
+
+        refresh:
+            For content that require variants. Find the default variant and render it again.
+
+        restart:
+            For content that require server threads. Toggle an unique environment variable and open the content, which activates a new server thread.
+
+        Returns
+        -------
+        Task | None:
+            A task for the content render when available, otherwise None
+
+        Raises
+        ------
+        RuntimeError
+            Found an incorrect number of default variants.
+
+        Examples
+        --------
+        >>> _re_whatever()
+        """
+        # Update the item to its current state.
+        # The 'app_mode' is not set until a bundle is created and deployed.
+        # During the deployment process, the 'app_mode' is read from manifest.json and written to the database.
+        # Until this occurs the 'app_mode' will be 'unknown'.
+        self.update()
+
+        if self.app_mode in {
+            "rmd-static",
+            "jupyter-static",
+            "quarto-static",
+        }:
+            variants = self._variants.find()
+            variants = [variant for variant in variants if variant.is_default]
+            if len(variants) != 1:
+                raise RuntimeError(
+                    f"Found {len(variants)} default variants. Expected 1. Without a single default variant, the content cannot be refreshed. This is indicative of a corrupted state."
+                )
+            variant = variants[0]
+            return variant.render()
+
+        if self.app_mode in {
+            "api",
+            "jupyter-voila",
+            "python-api",
+            "python-bokeh",
+            "python-dash",
+            "python-fastapi",
+            "python-shiny",
+            "python-streamlit",
+            "quarto-shiny",
+            "rmd-shiny",
+            "shiny",
+            "tensorflow-saved-model",
+        }:
+            random_hash = secrets.token_hex(32)
+            key = f"_CONNECT_RESTART_TMP_{random_hash}"
+            self.environment_variables.create(key, random_hash)
+            self.environment_variables.delete(key)
+            # GET via the base Connect URL to force create a new worker thread.
+            url = urls.append(dirname(self.config.url), f"content/{self.guid}")
+            self.session.get(url)
+            return None
+
+        warnings.warn(
+            f"Content '{self.guid}' with application mode '{self.app_mode}' does not require restarts"
+        )
+        return None


@mmarchetti - I am adding you for this section in particular. This is the crux of the change, and I want to check if the business logic is reasonable. The big caveat to this approach is upgrading the app_mode checks over time to accommodate new application modes. I've been tinkering with adding information to the warning message that directs users to open a GitHub issue as a lazy way to know if this happens.

The big caveat to this approach is upgrading the app_mode checks over time to accommodate new application modes.

We ran into the same thing in connect_api. I wonder if we ought to have a more central place to retrieve this (either on the server, that enumerates what it knows about), or somewhere common in our code we could depend on. Of course, we don't need to / shouldn't solve that here, but it's not unique to this SDK!

toph-allen

Looks great! ✨

I don't have any specific feedback that should prevent merging — just running thoughts on stuff we've been discussing.

src/posit/connect/content.py

toph-allen · 2024-07-22T20:05:10Z

src/posit/connect/content.py

+        Submit a refresh request to the server for the content. After submission, the server executes an asynchronous process to refresh the content. This is useful when content is dependent on external information, such as a dataset.
+
+        See Also
+        --------
+        restart
+
+        Notes
+        -----
+        This method is identical to `restart` and exists to provide contextual clarity. Both methods produce identical results. When working with documents, natural language prefers "refresh this content" instead of "restart this content" since documents do not require a system process. When writing software that operates on multiple types of content (e.g., applications, documents, scripts, etc.), you may use either 'refresh' or 'restart' to achive the same result.


I like this way of discussing it.

src/posit/connect/content.py

nealrichardson

Cool! A few notes.

integration/resources/bundles/example-flask-minimal/hello.py

integration/tests/posit/connect/test_env.py

nealrichardson · 2024-07-22T20:26:26Z

src/posit/connect/content.py

+
+        Notes
+        -----
+        This method is identical to `refresh` and exists to provide contextual clarity. Both methods produce identical results. When working with applications, natural language prefers "restart this content" instead of "refresh this content" since applications require a system process. When writing software that operates on multiple types of content (e.g., applications, documents, scripts, etc.), you may use either 'restart' or 'refresh' to achieve the same result.


I don't know that "restart" and "refresh" should be identical. I might think you have restart(), which works on interactive content (and errors on static), and render(), which works on static (and errors on interactive), and then refresh() is the generic. Then you have

def refresh(self, **kwargs): if self.is_interactive(): self.restart() else: self.render(**kwargs) # for variants/params

That way, if you want to be strict about what you're intending (restart or render), you can.

If you want to proceed in the current direction, you don't need _re_whatever(), just define one and have the other call it, you don't need them both to call some third function.

After discussing, we decided to implement restart and `render' and provide a guide error message if the content type does not support the invoked method.

nealrichardson · 2024-07-22T20:32:05Z

src/posit/connect/content.py

+    @property
+    def _variants(self) -> Variants:
+        return Variants(self.config, self.session, self.guid)


We'll add a v1 version, but even so, you'll still have to fall back to the old APIs when you're on old (well, contemporary) Connect. This is fine for this PR, since I'm guessing you're not implementing all of the Variants features (I haven't read that far yet), but just wanted to put that out there for the future. It's been in production for years, people use it, so our notion of internal vs. public doesn't really matter.

src/posit/connect/env.py

src/posit/connect/content.py

Co-authored-by: Jonathan Keane <[email protected]>