Skip to content

Conversation

seun-ja
Copy link
Contributor

@seun-ja seun-ja commented Aug 29, 2025

Aims to resolve #3256

Relaxes rules to accommodate LLM models. Also includes tests and updates current ones

Copy link
Contributor

@vdice vdice left a comment

Choose a reason for hiding this comment

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

I'm not sure if this is the ideal way to go about the fix. Here's my initial thought:

With the current version of this PR, all keys/values in the manifest are now exempt from the exceptions added here. This is problematic specifically for Spin application names (and maybe for other keys; I haven't yet looked in depth). We have logic elsewhere (eg spin new and spin deploy) with more restrictive naming rules, which I think we want to continue to uphold. For instance, admitting the : character in an app name would cause its OCI artifact reference to no longer be valid, and spin deploy would fail.

I'm not quite sure about the genesis of forbidding a segment from starting with a number, though @lann may recall. It's possible that is a restriction that could be lifted...

Hopefully @lann (or @itowlson?) can advise here as I'm still familiarizing myself with the code. My naive thought was we'd update logic (either in the serde crate here or maybe the toml parser?) to specifically ignore ai_models string values. Or, if ignoring is too lax, basically just apply the exceptions you've added here to these string values, but continuing to preserve the default restrictions for everything else in the manifest.

@fibonacci1729
Copy link
Collaborator

At the moment, I don't see much value in restricting the IDs of ai_models to be kebab-cased (iirc this was put into place in anticipation of value imports in the component model which won't land for a longish time). I think we can just use String in lieu of KebabId here and remove the compat enforcement here.

@itowlson
Copy link
Collaborator

@seun-ja Unless there's value in having the history (or it's hard to do) I'd suggest squashing the commits here.

@seun-ja seun-ja force-pushed the manifest-ai-model-parsing-too-restrictive branch from 3fd3e77 to abc280d Compare September 1, 2025 10:47
@seun-ja seun-ja changed the title Adds support for special case : with test Adds support for special case Sep 2, 2025
@seun-ja seun-ja requested a review from vdice September 2, 2025 12:01
Copy link
Contributor

@vdice vdice left a comment

Choose a reason for hiding this comment

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

Nice, love how simple this fix turned out to be! If it is a quick add, should we add unit test coverage?

@vdice vdice requested a review from fibonacci1729 September 2, 2025 16:48
Copy link
Collaborator

@fibonacci1729 fibonacci1729 left a comment

Choose a reason for hiding this comment

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

Looks good to me! Maybe just update the title to reflect the new changes.

@itowlson itowlson changed the title Adds support for special case Allow non-kebab-case names in component ai_models Sep 2, 2025
@itowlson
Copy link
Collaborator

itowlson commented Sep 2, 2025

@seun-ja Could you GPG sign please? Thanks!

Some LLM model names contain colon or numbers after a hyphen which was previously rejected
by the manifest parser. This change allows these special cases.

Signed-off-by: Aminu Oluwaseun Joshua <[email protected]>
@seun-ja seun-ja force-pushed the manifest-ai-model-parsing-too-restrictive branch from abc280d to 5599534 Compare September 2, 2025 20:58
@seun-ja
Copy link
Contributor Author

seun-ja commented Sep 2, 2025

@itowlson GPG signed.

Out of curiosity, could it be because I squashed my commit that could have made it seem like I didn't GPG sign? The commit even passed the DCO check

@itowlson
Copy link
Collaborator

itowlson commented Sep 2, 2025

@seun-ja Oh, that's possible. Squashing would likely have retained the signed-off-by text which is the DCO, but if you're manually GPG-signing then it's a new commit and so presumably waves hands a new cryptographic thingummy needs to be whatsited.

If you want, you can turn on automatic GPG signing by setting commit.gpgsign=true in your git config - I have this and so I never have to think about whether I need to re-sign after a merge/rebase.

Anyway thanks for doing it, I see the Verified flag and I've queued an auto merge.

@itowlson itowlson enabled auto-merge September 2, 2025 21:07
@itowlson itowlson merged commit 45f153f into spinframework:main Sep 2, 2025
17 checks passed
@seun-ja seun-ja deleted the manifest-ai-model-parsing-too-restrictive branch September 2, 2025 22:25
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.

manifest: ai_model parsing too restrictive
4 participants