feat: enforce that a version must be specified#544
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.
Something seems to have gone wrong for the cards? They should be templates, but this PR makes them filled.
| @@ -1,56 +1,25 @@ | |||
| --- | |||
| library_name: kernels | |||
| {% if license %}license: {{ license }} | |||
There was a problem hiding this comment.
Shouldn't the license be filled from the metadata, I don't think we want to hardcode it?
There was a problem hiding this comment.
yea 100% I made the mistake of pushing hydrated cards 🤦♂️, fixed in latest changes
|
|
||
| kernel_module = get_kernel("{{ repo_id }}") | ||
| {{ functions[0] }} = kernel_module.{{ functions[0] }} | ||
| kernel_module = get_kernel("kernels-test/cutlass-gemm-tvm-ffi", version=1) |
There was a problem hiding this comment.
I am confused, this should be done by card filling, right? The card should be filled at upload time, not in the repo already.
There was a problem hiding this comment.
same as above, should be resolved in latest changes
| - `{{ layer }}` | ||
| {% endfor %} | ||
| {% endif %} | ||
| - `cutlass_gemm` |
There was a problem hiding this comment.
Same here. Now the list of functions would never update as a kernel is extended.
| return revision | ||
| elif version is not None: | ||
|
|
||
| if version is not None: |
There was a problem hiding this comment.
We can keep this an elif right? To indicate that this is a continuation of the previous condition.
There was a problem hiding this comment.
yes that makes more sense syntactically, updated to use elif in the latest changes
| result = activation.relu(out, x) | ||
| ``` | ||
| """ | ||
| if revision is None and version is None: |
There was a problem hiding this comment.
Why do we need this here? select_revision_or_version gets called below. We should have one place for validation, not the same validation in multiple places.
There was a problem hiding this comment.
good catch, removed in latest
| if revision is not None and version is not None: | ||
| raise ValueError("Either a revision or a version must be specified, not both.") | ||
| if revision is None and version is None: | ||
| raise ValueError("Either a revision or a version must be specified.") | ||
|
|
There was a problem hiding this comment.
Here we need the duplication because the fetching is only lazy (for good reasons).
3 participants
This PR enforces that a version or revision must be specified in all cases
note: this PR also updates the examples cards to render correctly; updates the
__all__s for some functions, adds a small change for the cli to work on tvm-ffi kernels, and regenerates the cards since there were template placeholders previously. I added these changes to this PR since I updated the card template to include the version and thought the examples should reflect.