Hint about extra fields in conda-forge.yml #1920

ytausch · 2024-05-03T18:08:38Z

Checklist

Added a news entry

This PR contains the functional changes of #1900, to be merged separately. See also parts of the discussion there.

TLDR: recipe-lint now hints about additional fields as compared with the Pydantic schema.

ytausch · 2024-05-03T18:15:22Z

conda_smithy/lint_recipe.py

@@ -1,6 +1,11 @@
 # -*- coding: utf-8 -*-

 from collections.abc import Sequence, Mapping
+from typing import List
+
+from pydantic import BaseModel


@isuruf Let's continue the discussion about adding pydantic here.

~~By the way: pydantic is already listed as a project dependency in environment.yml~~
Edit: This comment does not contribute to the discussion, my mistake.

For clarity pydantic is a development-only dependency. So that only affects development, not installation. We generate a JSON schema for runtime use. For context, it was added in PR: #1756

Is it possible to use jsonschema for this case too?

I understand that pydantic is not currently a part of https://github.com/conda-forge/conda-smithy-feedstock/blob/main/recipe/meta.yaml dependencies.

I am not aware of a good way of doing this with jsonschema.

Also, I want to repeat my argument:

I would like and strongly propose to keep pydantic here. Pydantic provides an elegant way to find additional fields that are not part of the schema (see my implementation) - and also, I think the pydantic model should be used in more parts of the smithy code anyway since it provides a type-safe way to access fields.

Why is adding pydantic as a new dependency so bad?

Why is adding pydantic as a new dependency so bad?

Please see my comment on the other thread.

I am not aware of a good way of doing this with jsonschema.

This is easy. Load the json schema and see if "additionalProperties": false is there for the attribute.

Please see my comment on the other thread.

@isuruf I just scrolled through every comment you left at my conda-smithy PRs but could not find an answer to my question. You said:

This adds a new dependency pydantic on users. Previously it was only a dev dependency. Is it possible to remove this?

and

I strongly disagree. This PR use pydantic to check for which fields to hint about. If we can't do that using the json schema we are essentially making a distinction between pydantic model and json schema.

I do not see how this explains why we want to avoid pydantic as a dependency. To your latter comment, I replied:

What do you mean with "distinction"? The pydantic model should be equivalent to the JSON schema anyway - if it's not, we have a bug.

This is easy. Load the json schema and see if "additionalProperties": false is there for the attribute.

This requires us to have a separate JSON schema that actually has additionalProperties set to false for the fields we want to warn about since we have agreed to allow additional properties in the main schema. Is this what you propose?

@isuruf I would really really like to solve this debate as I want this PR to get merged.

ytausch · 2024-05-05T10:15:20Z

Note: The signature of lintify_forge_yaml changed but makes a lot more sense now since not all lints or hints must be JSON validation-related. Formally, this would require a deprecation process but I want to raise the question of who actually uses this method.

We had a discussion about deprecation in #1906

ytausch · 2024-05-10T11:51:02Z

What is the status of this PR? @conda-forge/core @isuruf

conda_smithy/lint_recipe.py

ytausch · 2024-05-27T16:24:33Z

@isuruf

ytausch · 2024-06-07T12:42:56Z

@isuruf There are still some open discussions about pydantic

don't warn for AzureRunnerSettings and CondaBuildConfig

ytausch · 2024-07-10T10:07:06Z

@isuruf You did not reply here since almost 2 months although I pinged you multiple times. Is there anything I need to do? Fine of course if you're currently busy, I'll wait here.

h-vetinari · 2024-11-21T13:48:22Z

Sorry for the very long delay here @ytausch! I stumbled over this due to #2152 and would be very interested to help you unblock this work, as it's necessary for a lint I'd like to add (#2155).

CC @beckermr

beckermr

The issue here is requiring pydantic at run time IIRC. We need to resolve this sticking point before we can merge.

ytausch · 2024-11-21T15:25:45Z

What exactly is the reason that we don't want to have pydantic as a runtime dependency?

isuruf · 2024-11-21T20:19:38Z

I thought I made my reasons clear in #1900. Let me know if you need more clarification.

h-vetinari · 2024-11-21T21:10:58Z

@isuruf: I thought I made my reasons clear in #1900. Let me know if you need more clarification.

Looking at that PR, the only relevant-looking thread I see is

@isuruf: This adds a new dependency pydantic on users. Previously it was only a dev dependency. Is it possible to remove this?

@ytausch: I would like and strongly propose to keep pydantic here. Pydantic provides an elegant way to find additional fields that are not part of the schema (see my implementation) - and also, I think the pydantic model should be used in more parts of the smithy code anyway since it provides a type-safe way to access fields.

@isuruf: I strongly disagree. This PR use pydantic to check for which fields to hint about. If we can't do that using the json schema we are essentially making a distinction between pydantic model and json schema.

@ytausch: What do you mean with "distinction"? The pydantic model should be equivalent to the JSON schema anyway - if it's not, we have a bug. I am not aware of a good way of doing this with JSON schema.

@ytausch: @isuruf Let's continue this in #1920.

So the sticking point is not having a runtime dependence on pydantic? Why is that an issue? I don't care much, but I've found that #2155 without this PR doesn't actually work. I definitely think fixing linting on wrong distro-values is more important than whether we depend on pydantic, but I'd also be happy if there's a fix for #2152/#2155 that works without the runtime dependence.

isuruf · 2024-11-21T21:29:47Z

So the sticking point is not having a runtime dependence on pydantic?

No, the main point is that I want the json schema to be complete.

If we can't do that using the json schema we are essentially making a distinction between pydantic model and json schema.

h-vetinari · 2024-11-21T21:39:34Z

No, the main point is that I want the json schema to be complete.

How do you imagine we encode the fact that additional fields are forbidden in the json schema? AFAICT json has no way to specify that, so whatever we do, we'd have to introduce a marker-field that says "this JSON schema cannot be extended". Which is what this PR is doing with "additionalProperties" AFAIU (naming can be bikeshod of course, e.g. __no_extend / __closed, etc.).

To me saying "json schema needs to be complete" thus becomes equivalent to "we shouldn't lint on unknown fields", which I strongly disagree with.

isuruf · 2024-11-21T21:45:25Z

How do you imagine we encode the fact that additional fields are forbidden in the json schema?

additionalProperties: false

h-vetinari · 2024-11-21T21:58:06Z

So the issue is just the choice of default? I.e. whether we need to add "additionalProperties": false to non-extensible schemas, or whether - as this PR seems to be doing - set "additionalProperties": true (?!) and build on that?

It'd be completely fine for me to opt into extra="forbid" with "additionalProperties": false; that would be a natural equivalence IMO. Is this what you had in mind?

isuruf · 2024-11-21T22:02:31Z

It'd be completely fine for me to opt into extra="forbid" with "additionalProperties": false; that would be a natural equivalence IMO. Is this what you had in mind?

Yes, but with a hint in the linter instead of a lint.

h-vetinari · 2024-11-22T09:22:58Z

@ytausch, now that we've elucidated Isuru's objection, do you think you'd be able to implement this request?

Also, @beckermr, in case you're still opposed to the runtime dependence on pydantic, could you explain the problem with that please?

beckermr · 2024-11-22T10:50:18Z

I don't care personally. I was trying to make sure more discussion was had since others had objected.

ytausch requested a review from a team as a code owner May 3, 2024 18:08

ytausch mentioned this pull request May 3, 2024

refactor conda-forge.yml linting logic, hint about extra fields #1900

Closed

1 task

ytausch commented May 3, 2024

View reviewed changes

isuruf reviewed May 11, 2024

View reviewed changes

conda_smithy/lint_recipe.py Show resolved Hide resolved

ytausch force-pushed the hint-extra-fields branch from 036d3df to 155b56e Compare May 13, 2024 14:44

ytausch requested a review from isuruf June 7, 2024 12:43

ytausch added 6 commits July 10, 2024 12:02

hint about extra fields in conda-forge.yml

8a6d442

add news file

0d5c730

use outdated List type

d6307ea

lintify_forge_yaml returns lists of str

294ce7e

use pytest, add tests for AzureRunnerSettings and CondaBuildConfig

1068125

make extra fields hint configurable

8a429ed

don't warn for AzureRunnerSettings and CondaBuildConfig

ytausch force-pushed the hint-extra-fields branch from 155b56e to 8a429ed Compare July 10, 2024 10:03

ytausch mentioned this pull request Jul 26, 2024

fix: regenerate schema #1993

Merged

h-vetinari added 2 commits November 22, 2024 00:26

Merge remote-tracking branch 'upstream/main' into hint-extra-fields

8b6efc0

fix inaccurate tests

6c0a9db

h-vetinari mentioned this pull request Nov 21, 2024

Linting conda-forge.yml does not diagnose (some) schema violations #2152

Open

h-vetinari force-pushed the hint-extra-fields branch from aca42c0 to 1a129f6 Compare November 21, 2024 13:44

h-vetinari mentioned this pull request Nov 21, 2024

ENH: lint incorrect distro names in os_version #2155

Draft

appease linters

bfc6a4b

h-vetinari force-pushed the hint-extra-fields branch from 1a129f6 to bfc6a4b Compare November 21, 2024 13:49

beckermr requested changes Nov 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hint about extra fields in conda-forge.yml #1920

Hint about extra fields in conda-forge.yml #1920

ytausch commented May 3, 2024 •

edited

Loading

ytausch May 3, 2024

ytausch May 3, 2024 •

edited

Loading

jakirkham May 3, 2024

ytausch May 5, 2024 •

edited

Loading

ytausch May 5, 2024

isuruf May 11, 2024

ytausch May 13, 2024 •

edited

Loading

ytausch May 21, 2024

ytausch commented May 5, 2024 •

edited

Loading

ytausch commented May 10, 2024

ytausch commented May 27, 2024

ytausch commented Jun 7, 2024

ytausch commented Jul 10, 2024

h-vetinari commented Nov 21, 2024

beckermr left a comment

ytausch commented Nov 21, 2024

isuruf commented Nov 21, 2024

h-vetinari commented Nov 21, 2024

isuruf commented Nov 21, 2024

h-vetinari commented Nov 21, 2024 •

edited

Loading

isuruf commented Nov 21, 2024

h-vetinari commented Nov 21, 2024

isuruf commented Nov 21, 2024

h-vetinari commented Nov 22, 2024

beckermr commented Nov 22, 2024

Hint about extra fields in conda-forge.yml #1920

Are you sure you want to change the base?

Hint about extra fields in conda-forge.yml #1920

Conversation

ytausch commented May 3, 2024 • edited Loading

ytausch May 3, 2024

Choose a reason for hiding this comment

ytausch May 3, 2024 • edited Loading

Choose a reason for hiding this comment

jakirkham May 3, 2024

Choose a reason for hiding this comment

ytausch May 5, 2024 • edited Loading

Choose a reason for hiding this comment

ytausch May 5, 2024

Choose a reason for hiding this comment

isuruf May 11, 2024

Choose a reason for hiding this comment

ytausch May 13, 2024 • edited Loading

Choose a reason for hiding this comment

ytausch May 21, 2024

Choose a reason for hiding this comment

ytausch commented May 5, 2024 • edited Loading

ytausch commented May 10, 2024

ytausch commented May 27, 2024

ytausch commented Jun 7, 2024

ytausch commented Jul 10, 2024

h-vetinari commented Nov 21, 2024

beckermr left a comment

Choose a reason for hiding this comment

ytausch commented Nov 21, 2024

isuruf commented Nov 21, 2024

h-vetinari commented Nov 21, 2024

isuruf commented Nov 21, 2024

h-vetinari commented Nov 21, 2024 • edited Loading

isuruf commented Nov 21, 2024

h-vetinari commented Nov 21, 2024

isuruf commented Nov 21, 2024

h-vetinari commented Nov 22, 2024

beckermr commented Nov 22, 2024

ytausch commented May 3, 2024 •

edited

Loading

ytausch May 3, 2024 •

edited

Loading

ytausch May 5, 2024 •

edited

Loading

ytausch May 13, 2024 •

edited

Loading

ytausch commented May 5, 2024 •

edited

Loading

h-vetinari commented Nov 21, 2024 •

edited

Loading