Skip to content

[build_rules] Add xls_dslx_multi_test rule #1012

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

mtdudek
Copy link
Contributor

@mtdudek mtdudek commented Jun 5, 2023

This commit adds new xls_dslx_multi_test rule.
It differes from xls_dslx_test by replacing compare with compare_list argument. This rule allows for easier DSLX testing, as it can replace multiple
xls_dslx_test targets that only differ on compare method.

Example:

xls_dslx_test(
    name = "rle_dslx_test",
    dslx_test_args = {
        "compare": "none",
    },
    library = "rle_dslx",
)

xls_dslx_test(
    name = "rle_dslx_ir_test",
    dslx_test_args = {
        "compare": "interpreter",
    },
    library = "rle_dslx",
)

can be reduced to:

xls_dslx_multi_test(
    name = "rle_dslx_test",
    compare_list: ["none", "interpreter"],
    library = "rle_dslx",
)

Corresponding issue #1011
CC @proppy

This commit adds new `xls_dslx_multi_test` rule.
It differes from `xls_dslx_test` by replacing `compare`
with `compare_list` argument. This rule allows for
easier DSLX testing, as it can replace multiple
`xls_dslx_test` targets that only differ on `compare` method.

Example:
```
xls_dslx_test(
    name = "rle_dslx_test",
    dslx_test_args = {
        "compare": "none",
    },
    library = "rle_dslx",
)

xls_dslx_test(
    name = "rle_dslx_ir_test",
    dslx_test_args = {
        "compare": "interpreter",
    },
    library = "rle_dslx",
)
```
can be reduced to:
```
xls_dslx_multi_test(
    name = "rle_dslx_test",
    compare_list: ["none", "interpreter"],
    library = "rle_dslx",
)
```

Signed-off-by: Maciej Dudek <[email protected]>
@proppy
Copy link
Member

proppy commented Jun 7, 2023

Maybe we could simply make xls_dslx_test take an additional string_list compare attr rather than adding a new rule, curious what others think?

@mtdudek mtdudek mentioned this pull request Jun 13, 2023
1 task
@mtdudek
Copy link
Contributor Author

mtdudek commented Jun 13, 2023

I chose to create new rule, so that old one doesn't become a rule to do it all.

@tmichalak
Copy link

To me a target named in way that indicates it actually runs mutliple tests makes sense. I do see that changes required to add an additional string_list are smaller than a new target, but the new rule could be extended in the future to allow for defining a test matrix with all possible combinations.

@proppy
Copy link
Member

proppy commented Jun 15, 2023

I'd love to get others opinion here, I'm worried that if we add another dimention to the rule namespaces (with the _multi suffix) we end up with too many `test rules https://google.github.io/xls/bazel_rules_macros/.

I also wonder if it needs to be implemented as new rules, are https://bazel.build/extending/macros to simply instanciate multiple rules with different parameters?

@cdleary
Copy link
Collaborator

cdleary commented Jun 21, 2023

Yeah I think you could put this as a local macro in a build_defs.bzl file for now.

We need to determine what the longer term plan is for comparing the DSLX interpreter results to JIT'd results / how ability or need to select an execution mode is surfaced to the user. Right now we force them to compare inline as an opportunistic way to ensure cross-checking testing, but from a UX perspective that's not really relevant, the user just wants their code to run, whether via JIT'd XLS IR or DSLX interpreter should largely just be a function of observed speed.

@proppy
Copy link
Member

proppy commented Apr 3, 2024

Closing following @cdleary suggestion, if this is needed by #1211 it can be implemented as a local https://bazel.build/extending/macros rather than introducing a new rule.

@proppy proppy closed this Apr 3, 2024
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.

4 participants