Deprecation: remove "use system packages" (python.system_packages config key and UI checkbox)

[humitos]

This option doesn't work on builds using build.os because the Python binary used it's not the one from the system, but another one compiled via asdf. This means that creating a virtualenv with access to the "system packages" has no effect at all.

This PR removes the feature completely from the UI and also from v1 and v2 config file.

📅 Merging date

This PR has to be deployed on August 29th, so merge it between August 23th and August 28th.

Closes #10500

---

📚 Documentation previews 📚

• User's documentation (docs): https://docs--10562.org.readthedocs.build/en/10562/

• Developer's documentation (dev): https://dev--10562.org.readthedocs.build/en/10562/

[humitos]

Thinking a little more about this, I think we should completely remove it from our code as well since it's broken when using new build.os images (it's not "just deprecated"). I checked the builds from the last 90 days and we have 640 projects using this feature that we should contact before removing it from the code: https://ethicalads.metabaseapp.com/question/228-projects-using-python-site-packages?days=90

So, I'd propose to follow the pattern we have been following:

• blog post
• email
• reach final date (let's say August 29th)
• delete the code

[ericholscher]

@humitos I do worry a bit about overwhelming the blog with deprecations. It would be nice to include any things in the config file at the same time, perhaps bundling this with the deprecation of the old build images?

[humitos]

I do worry a bit about overwhelming the blog with deprecations.

I understand your point about this, but I think I have a different point of view here. I believe small changelogs like this one are good usage of our blog. In any case, this is a conversation that's outside the scope of this PR and we can continue in another place.

It would be nice to include any things in the config file at the same time, perhaps bundling this with the deprecation of the old build images?

This may be a good idea, since it's related with the config file itself. However, the "big news" here will be "build.image is deprecated" which won't catch the attention of users using python.system_packages and may skip reading this blog post. On the other hand, users using build.os with python.system_packages will read a blog that's 80% unrelated to them. So, there is a trade off on both paths.

[ericholscher]

It would be nice to include any things in the config file at the same time, perhaps bundling this with the deprecation of the old build images?

This may be a good idea, since it's related with the config file itself. However, the "big news" here will be "build.image is deprecated" which won't catch the attention of users using python.system_packages and may skip reading this blog post. On the other hand, users using build.os with python.system_packages will read a blog that's 80% unrelated to them. So, there is a trade off on both paths.

I see them as related, at least because folks using system_packages are implicitly depending on the system deps installed on the build image. So it seems like they make sense to bundle together -- also partially just to reduce the total number of deprecations we have to do.

Overall I guess focusing each deprecation on 1 specific issue is probably best for clarity of communication.

[humitos]

I updated the PR to remove the code completely from our application. I will start writing the email, blog post and script to communicate this deprecation to our users.

[ericholscher]

We should put the date to merge this in the description:

## Merge date: August 29th

This PR looks good, but now users with a python.system_packages key in their settings will just get a generic error right? It feels like we should be linking them to the deprecation blog post and helping them understand the issue, at least?

If that is a lot of work, we can skip it, but I think as a user it's not great UX to have the error message like the setting never existed after this deprecation. I imagine there are example .readthedocs.yaml files in various places online that specify this setting, which people will try to use in the future, for example.

[humitos]

This PR looks good, but now users with a python.system_packages key in their settings will just get a generic error right? It feels like we should be linking them to the deprecation blog post and helping them understand the issue, at least?

This is a good point. It's currently showing,

I'll see if I can make that message better in a not so-complex way 😄

[humitos]

I was able to show custom messages based on the config key that failed and it wasn't too complex. Besides, it prepares the code for #9008 and similar issues.

It looks like the following now:

We cannot have links on the URL due to a Knockout.js in the frontend. I left a comment there so we can come back to improve those links in another iteration 😄

[ericholscher]

This looks like a good refactor, but I got a bit confused with the structure and what the various template variables are meant to be.

[ericholscher]

What error attribute? On the build itself?

[humitos]

Yes

[agjohnson]

If you would like this, we should make the APIv3 response match what we want, perhaps applying the filter on the field in the response there.

[humitos]

Hrm.. I don't think the API should return blob HTML tags on its response.

[agjohnson]

Similar to templates and HTMX, the front end should only have to display the data from the backend application. I don't think we should have a front end reimplementation of urlize, which is required here to show build.error in the same way the templates would render it.

On the implementation side, I think we should be authoring error message to be a bit more rich than what we're doing now. Relying on urlize results in error message HTML like:

Something happened. See our documentation on webhooks at <a href="https://docs.readthedocs.io/page/something.html">https://docs.readthedocs.io/page/something.html</a>

This is not super user friendly, but is also not accessible either. Using HTML in the error would be better, but does require some translation to HTML at some point:

Something happened. <a href="https://docs.readthedocs.io/page/something.html">Read our documentation on webhooks</a> for more information

This is perhaps another good point for the notification redesign.

[humitos]

Similar to templates and HTMX, the front end should only have to display the data from the backend application

I think this is out of discussion. We are not using HTMX which requires specific endpoints for its purpose. Besides, here we are talking about a resource API that exposes data to our users. If we want to start exposing "data to be rendered by the frontend without any modification" that's a whole different use case than what the API was thought for.

In the case we want move in that direction,we will need to add _display fields (or similar) to all the things we want to expose to user as-is.

[agjohnson]

I know we're not using HTMX, but we've talked about this approach specifically. Templates are more applicable then, and maintain a similar opinionated approach. My point is that we should do what we can at the backend in order to keep our frontend footprint low.

I think we talk about this more as part of the notification refactor either way. I just brought this up here to note that replicating urlize in front end code is not a great path forward.

If we want to start exposing "data to be rendered by the frontend without any modification"

I don't feel I'm talking about the same thing here. I am only referring to text with HTML links embedded, not any more structure than that.

Besides, here we are talking about a resource API that exposes data to our users

Well, I'd certainly say our API is dual function. The API needs to be useful for our own application too, it's not just a user resource API.

that's a whole different use case than what the API was thought for

Perhaps, but our application is also a consumer of this API. We don't need all the features exposed to end users though.

But if not the current API, it should be something on the application side. This can be separate, a different API or API-like endpoint, but it should be part of the application. I guess I don't see a strong reason for avoiding APIv3 though.

In the case we want move in that direction,we will need to add _display fields (or similar)

That seems fine, I don't have strong opinions on structure. We're not solving this problem here, so it's worth thinking about more. I'm just noting here as I think we should change our patterns, and the pattern noted in the PR here, in our notification refactor.

[ericholscher]

I agree this code could be improved with a refactor, but this PR pushes us forward in a useful way, so I'm 👍 on the incremental step, and we can think more deeply about notifications in the project for that.

[shoyer]

Just today I noticed that my project's doc builds are failing with the confusing error Invalid configuration option "python.system_packages". This configuration key has been deprecated and removed.

This was surprising because my config file already explicitly set system_packages: false, which is the new default behavior. Removing this line fixes my build, but I expected I would already have this behavior already...