Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
danieldk
left a comment
danieldk
left a comment
There was a problem hiding this comment.
Really useful functionality! I would make it more general though, because as it is now, we would have to deprecate this again if we add archived/deprecation in the future.
|
|
||
| from kernels.compat import tomllib | ||
|
|
||
| REDIRECT_FILENAME = "REDIRECT.toml" |
There was a problem hiding this comment.
I think we should have a more generic name than that, so that we can use the same file for multiple signaling purposes.
Maybe kernel-status.toml?
| @dataclass | ||
| class RedirectInfo: | ||
| destination: str | ||
| message: str |
There was a problem hiding this comment.
Redirect should be one of a union (so that we can also have e.g. deprecated/unmaintained in the future).
| message: str | ||
|
|
||
|
|
||
| def parse_redirect_file(content: str) -> RedirectInfo: |
There was a problem hiding this comment.
I think we should have this similar to https://github.com/huggingface/kernels/blob/main/kernels/src/kernels/metadata.py where these are methods.
Maybe we should consider switching to pydantic for these deserialization/ validation purposes.
| RED_COLOR = "\033[91m" | ||
| RESET_COLOR = "\033[0m" | ||
| msg = f"\n{RED_COLOR}{msg}{RESET_COLOR}" |
There was a problem hiding this comment.
Do these work well in Windows shells, I vaguely remember ASCII sequences giving issues and you need a library to make them work on *nix and Windows.
| if "@" in redirect.destination: | ||
| return redirect.destination.rsplit("@", 1) | ||
| return redirect.destination, "main" |
There was a problem hiding this comment.
@ in the string data seems very ad-hoc if we have a structured data format like TOML?
| backend: str | None = None, | ||
| variant_locks: dict[str, VariantLock] | None = None, | ||
| user_agent: str | dict | None = None, | ||
| follow_redirects: bool = True, |
There was a problem hiding this comment.
I would leave this out. We already have plenty of arguments and we are probably going to add framework soon. I think we should just go for doing the right thing by default. If a kernel developer sets a repo to redirect, they probably really want to redirect.
| @dataclass | ||
| class RedirectInfo: | ||
| destination: str | ||
| message: str |
There was a problem hiding this comment.
I would leave out a message for now.
3 participants
This PR adds a concept of redirection via a
REDIRECT.tomlfile on themainbranch.The kernel tool will now first check for a redirection file, read and apply the redirection if it exists. The core use case is enable a way to manually redirect users from old repos to newer kernel repos with a custom message.
Note: redirections are single hop only, its not possible to redirect to another redirect
example file
this example file is currently hosted https://huggingface.co/drbh/test-kernel-redirect