[Tools] Improving integration test system #314
Replies: 8 comments 19 replies
-
|
I need to metabolize this (unfortunately my mind is somewhere else right now). However, I have two small comments over the runner. These comments are outside of the scope of the configuration file, but somehow related to the development of the CI/CD.
|
Beta Was this translation helpful? Give feedback.
-
|
I haven't actually read @shundroid's new speculation integration test approach yet, but I think this is probably a very nice example for you guys to road-test this proposal? |
Beta Was this translation helpful? Give feedback.
-
|
I created another integration test script for speculation, based on @ebosnjak's original one. Speculation requires a different compilation flow (starting from the CF dialect, applying the speculation pass and a custom buffer placement pass, and using a new backend). I prefer using a configuration file over a custom integration test script, also because speculation tests can easily integrate into the existing CI/CD pipeline. However, my main concern is the growing number of configuration parameters. As an alternative, I'm also fine with placing a custom compilation flow script in each speculation integration test folder (e.g., {
"single_loop": {
"custom-compilation-flow": true
}
} |
Beta Was this translation helpful? Give feedback.
-
|
I would suggest a few enhancements to how things are captured in the configuration file. In the following, I assume Dynamatic is invoked like I would also like to separate clearly in this discussion capability from policy: the existence of a feature (a capability) does not imply that we will use it for standard testing (a policy); yet, the absence of one of the fundamental and typical features of such a "language" may mean that tomorrow it will be abandoned when someone needs it or when a new policy is required. This post is only about capability, not policy.
"gcd": {
"option": "--buffer-algorithm fpga20",
},
"binary_search": {
"option": "--timeout 1000",
},
"*": {
"option": "--buffer-algorithm fpl22",
"option": "--timeout 200",
},
"memory/*": {
"option": "--timeout 30",
},I guess in JSON there is an issue with two keys named
"option": "--buffer-algorithm fpga20",
"option": "--timeout 200",
"__DEFAULT": {
"option": "--buffer-algorithm fpl22",
"option": "--timeout 200",
},
"__MEMORY_DEFAULT": {
"option": "--timeout 30",
},
"gcd": {
"option": "--buffer-algorithm fpga20",
},
"binary_search": {
"option": "--timeout 1000",
},
"*": {
"macro": "__DEFAULT",
},
"memory/*": {
"macro": "__DEFAULT",
"macro": "__MEMORY_DEFAULT",
},Note that
Whether this is good policy or not, it is open for discussion. I think it may be handy that a missing include is reported as a warning but not considered an error.
"gcd": {
...
"label": "pull",
"label": "performance",
"label": "nightly",
"label": "lsq",
},
"binary_search": {
...
"label": "performance",
"label": "nightly",
},
"memory/*": {
...
"label": "pull",
"label": "nightly",
"label": "lsq",
},The one could run
"__BUFFER_LANA": {
"option": "--buffer-algorithm fpga20",
},
"__BUFFER_CARMINE": {
"option": "--buffer-algorithm fpl22",
},
"__CONVERSION_LANA": {
"option": "--conversion cfg",
},
"__CONVERSION_AYA": {
"option": "--conversion ftd",
},
"__DEFAULT_PULL": {
"macro": "__BUFFER_LANA",
"macro": "__CONVERSION_LANA",
"label": "pull",
},
"gcd": {
"macro": "__DEFAULT_PULL",
"label": "gcd_basic",
},
"gcd": {
"macro": "__BUFFER_CARMINE",
"macro": "__CONVERSION_AYA",
"label": "gcd_advanced",
"label": "test_ftd",
},Since they entries have no name and the source files are the same, one need to rely on the existence of labels to identify them. Is it wise? I guess it is debatable.
"gcd": {
"macro": "__DEFAULT_PULL",
"macro": "__CONVERSION_AYA",
"label": "gcd_paolo_stuff",
"nolabel": "pull",
"nooption": "--timeout",
},The semantic should be defined properly but at worst it could be "if there is a label
"*": {
"command": "dynamatic %option%",
},
"__SPECULATION": {
"command": "my_script %option%",
"label": "speculation",
},
"single_loop": {
"macro": "__SPECULATION",
"label": "shun_private",
} |
Beta Was this translation helpful? Give feedback.
-
|
Hi All! I have a few points, splitting them out to make replies easier.
It is my understanding that, currently, this is not the case. The main The closest thing currently possible to the above is to write all commands that the # Write .dyn script with appropriate source file name
dyn_file = DYNAMATIC_ROOT / "build" / f"test_{id}.dyn"
write_string_to_file(SCRIPT_CONTENT.format(src_path=c_file), dyn_file)
# ... snip ...
with open(Path(out_dir) / "dynamatic_out.txt", "w") as stdout, \
open(Path(out_dir) / "dynamatic_err.txt", "w") as stderr:
# Run test and output result
exit_code = run_command_with_timeout(
DYNAMATIC_COMMAND.format(script_path=dyn_file),
timeout=timeout,
stdout=stdout,
stderr=stderr
)I agree that this is not optimal. |
Beta Was this translation helpful? Give feedback.
-
|
Also @shundroid - sorry to keep mentioning your speculation stuff here. This is not particular to your work, it is just the only other thing I have a rough enough idea about to use it as an example. |
Beta Was this translation helpful? Give feedback.
-
|
I cannot say that I have a full understanding of the problem at hand, but I agree there is a looming problem and I like what I read. I have been worried for a while that we should have (a) one stable and usable HLS tool, usable by anyone, and that we do have (b) 75 different research ideas that are implemented inside and around it; realistically, only a minor fraction of the latter ones will deserve to become part of the former. Articulating the relations between these different elements is essential to us. I am largely unable to, but the above seems to go in the right direction, in my view. |
Beta Was this translation helpful? Give feedback.
-
I think that Dynamatic, being a research project, will always be a mixture of (a) and (b) - and that is OK, as long as there is an easy way to access the (a) part. I don't know the project that well, but I assume that there will always be aspects of the project that fundamentally cannot be made compatible, and it is OK for them to exist in parallel as long as the flows to use them are clearly defined and continuously tested. Some more thoughts on this, that are now really well beyond the scope of CI/integration testing: On the other hand, I think it might be good, moving forward, to think about and have a clear policy on what should actually live in this repository/be merged into this repo. I will take my XLS backend as an example: It is an experiment, and is not properly usable. I make no secret of that fact. I actually tried to convince @Jiahui17 and @Carmine50 not to merge it in. Their argument, which I fully understand, is that it is better to merge it in so you have it. It can always be deleted if it starts causing problems down the road. However, in my experience, it is very hard to actually delete code: You, effectively, need a sign-off from everyone using Dynamatic that it is OK to get rid of. With it in place (and assuming it is tied into CI, which it is not yet), any change to the handshake dialect definition/structure of handshake_export.mlir will break this backend and red lights to flash on the PR. That means, if such a change needs to be made in 6 months when I might not be available anymore, you are left with the following choices:
If, instead, the XLS backend was simply left behind on an old branch or even a different repo, it will not interfere with new development. Should, down the road, someone decide that they want to use it, they can use the old version of dynamatic. If they want to use it with a more modern version of Dynamatic, then the discussion can be had on if it is worth updating it. But at least the effort spent on updating is not done speculatively because someone might want it in the future, and there is someone with at least enough knowledge of what this thing is to be interested in it. Of course, I am not saying that nothing but core features should ever be merged, but I think it is worth a discussion on a per-feature basis. By merging it in, you are taking responsibility for it to be maintained and carried forward. Finally, I think it would be very good to try and shy away from any (breaking) changes to the core handshake dialect that are not directly tied to actual improvements in Dynamatic proper. Especially for "peripheral" projects like the one above. The handshake dialect is the "foundation" that makes sure all the Dynamatic Lego pieces actually fit together. Besides questioning if such changes are needed, we should consider if they are done in the least-invasive way possible. For example, adding an extra optional attribute that other passes can simply ignore is much better than replacing 3 handshake operations with fundamentally different ones. The distributed nature of the development here makes it difficult, but it would be very important if there is a basic process for such breaking changes. Maybe their design should be discussed in a biweekly meeting before being implemented, let alone merged? It is no fun if someone has been hard at work, developing a big new feature, and every two weeks it becomes incompatible with Dynamatic because the dialect is being changed at will. |
Beta Was this translation helpful? Give feedback.
-
|
Thanks @schilkp for this discussion. While I find I am ill-prepared to answer most of the conundrums, I think it is an important discussion to have and maybe @lana555 should eventually chime in.
I agree with all this. I would only like to comment that at this early stage of the project, occasionally, it may be appropriate to annoy all the developers to change some early bad decision that otherwise would stay conceptually broken forever. It should not happen every two weeks and it must be closely watched and carefully executed--the biweekly meetings have been introduced mainly to discuss when this is worth it, so we perfectly agree. 😄 |
Beta Was this translation helpful? Give feedback.
-
|
Great. Sorry to "parachute in" with a wall of text/brain-dump. It is a different issue than integration testing, but the decision as to "what actually is Dynamatic" is fundamental to "what parts of Dynamatic should be tested and how"
Absolutely - and in my eyes the ability to do this fairly easily is the strength of a research project as opposed to a commercial product. |
Beta Was this translation helpful? Give feedback.
-
To be fully JSON "compliant"1, the top level needs to be either a Still, many JSON parses would allow it, and simply implicitly add the enclosing I personally would not rely on it.
Yes this is also unfortunately not allowed in JSON. It would have to be an "*": {
"options": [
"--buffer-algorithm fpl22",
"--timeout 200"
]
},For something that primarily needs to be hand-written, I personally prefer YAML - but deciding on a serialization/config format tends to be as divisive as picking between a certain two text editors, so I am sure that others will have other preferences. Footnotes
|
Beta Was this translation helpful? Give feedback.
-
|
I am slightly worried that forcing everything into a declarative format (like a json file) is not very future proof: What if we want to add a style of integration test in the future that does not fit into the datamodel we define for this setup? We would have to re-write the integration setup from scratch. Also I worry that it would effectively amount to writing a custom json-dsl, which makes the setup very hard to learn for new contributors. While I appreciate the simplicity of adding a new integration test by writing a few lines of JSON, I would prefer a setup that makes adding basic integration tests simple while not preventing more complicated setups. I get the feeling that we are re-inventing the wheel here. There are existing test frameworks out there that have seen a lot of development. Would it be possible to just use one of them? We could simply define each integration test as a small function that calls the relevant tools with the relevant inputs in one such framework. To make this simple, we could define the parameters as classes/types, and provide utility functions that abstract away the repeated code. Typical integration test frameworks would provide the following benefits:
As a rough example, here is what I imagine this could look like. Note that I used GoogleTest because I am familiar with it from XLS, but it is not the only choice. Still - I really would push for a C++ testing framework. Everything that follows is rough pseudo code to give you an idea. I did not test it. TEST(BasicIntegrationTests, GCD) {
DynamaticFlags flags = DynamaticFlags(); // Use default flags
RunIntegrationTest("gcd.c", flags);
}Here is how some of the points mentioned above could map to this framework:
Tests can pull in the default flags and override them at will, or even define their own flags from scratch: TEST(BasicIntegrationTests, gcd) {
DynamaticFlags flags = DynamaticFlags(); // Use default flags
flags.buffer_algo = "fpga20";
RunIntegrationTest("gcd.c", flags);
}
TEST(BasicIntegrationTests, binary_search) {
DynamaticFlags flags = DynamaticFlags(); // Use default flags
flags.timeout = 1000;
RunIntegrationTest("binary_search.c", flags);
}
TEST(BasicIntegrationTests, schilk_special) {
DynamaticFlags flags = {
.buffer_algo = "schilks_secret_buffer_algo",
.timeout = -1,
...
}
RunIntegrationTest("top_secret.c", flags);
}
The simple approach is a loop: TEST(BasicIntegrationTests, gcd_sweep) {
for (auto buffer_algo: { "fpga20", "fpl22" }) {
DynamaticFlags flags = DynamaticFlags(); // Use default flags
flags.buffer_algo = buffer_algo;
RunIntegrationTest("gcd.c", flags);
}
}However, this would run sequentially. Google test provides a framework for parametric tests that allows a set of parameters to be applied to multiple test suites and the individual test/parameter pairs to run in parallel. The following sweeps both class SweepBufferAlgoFixture : public testing::TestWithParam<std::string> {
};
TEST_P(SweepBufferAlgoFixture, gcd) {
std::string buffer_algo = GetParam();
DynamaticFlags flags = DynamaticFlags();
flags.buffer_algo = buffer_algo;
RunIntegrationTest("gcd.c", flags);
}
TEST_P(SweepBufferAlgoFixture, binary_search) {
std::string buffer_algo = GetParam();
DynamaticFlags flags = DynamaticFlags();
flags.buffer_algo = buffer_algo;
RunIntegrationTest("binary_search.c", flags);
}
INSTANTIATE_TEST_SUITE_P(
BufferAlgoSweep,
SweepBufferAlgoFixture,
testing::Values("fpga20", "fpl22")
);
Gtest organizes tests into test suites that can be run separately. By default all tests are run, but if we have a known-broken test, It is also possible to skip tests dynamically. For example a test relying on gurobi could be skipped if gurobi is not available.
This maps nicely to c++ inheritance. Imagine the following structure that roughly mimics the tests you have described: └── integration_tests
├── binary_test.c
├── gcd.c
├── memory
│ ├── some_mem_related_test.c
│ └── TEST_SUITE.c
└── TEST_SUITE.cc(The In TEST(BasicIntegrationTests, gcd) {
DynamaticFlags flags = DynamaticFlags(); // Use default flags
flags.buffer_algo = "fpga20";
RunIntegrationTest("gcd.c", flags);
}In class MemoryDynamaticFlags : public DynamaticFlags {
public:
MemoryDynamaticFlags() {
// All memory tests use timeout 300
timeout = 300;
}
virtual ~MemoryDynamaticFlags() = default;
};
TEST(MemIntegrationTests, some_mem_related_test) {
DynamaticFlags flags = MemoryDynamaticFlags(); // use memory-specific flags
RunIntegrationTest("some_mem_related_test.c", flags);
}
Google test provides a test filter flag:
See loops/parametrics above. Regarding benchmarking: I don't know the specifics well enough to provide a quick example, but I hope you could imagine how defining such test flows can easily be done in gtest. Let me know what you think :)
|
Beta Was this translation helpful? Give feedback.
-
|
Thanks @schilkp for this thoughtful message. What Paolo said makes perfect sense from a functional point of view; since everyone faces the same issues, it makes sense to adopt a standardized model. Also, the fact that this tool is used by the tools you mentioned is a proof of reliability. I guess that, with such an approach, @shundroid should write his version of |
Beta Was this translation helpful? Give feedback.
-
|
I think this conversation is moving in a nice direction (that I cannot contribute super much to) but just chiming in to echo @schilkp's comment-
This is exactly what happened with the export-rtl.json file and what speculation needed. For a specific direction- putting the tests in c++ seems really nice to me, but I have basically no experience with this kind of stuff. |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for the great idea! I've used GoogleTest for C++ testing, and I remember it worked well. I have a small concern, which I'll post separately.
I think so. And in addition to that, instead of calling the method directly in Dynamatic’s C++, we’d spawn a process and verify the result in the C++ test file—just like we do in the Python script, right? I haven’t used GoogleTest for this kind of setup before, but I assume it should work fine.
Yeah, we’re pretty much fed up with the JSON-based declarative approach for a supposedly flexible backend. Lesson learned! 😂 |
Beta Was this translation helpful? Give feedback.
-
Please see my somewhat long and rambly response to paolo's response to my first response (...?$#!). In short: I think this is exposing a much more fundamental problem with the dynamatic project structure, because there is not one compiler - but many very related but subtly different ones. For now - yes, Shun could write his own Does that make sense? |
Beta Was this translation helpful? Give feedback.
-
That is the easiest way - yes. But both are possible. |
Beta Was this translation helpful? Give feedback.
-
|
@schilkp Thank you for your great idea! I have a quick question about tagging/grouping tests in GoogleTest. I know For example, is there a way for each test to have multiple labels if we use --gtest_filter or other possible methods? Just for reference for other people, people here seem to appreciate flexible tagging/grouping:
|
Beta Was this translation helpful? Give feedback.
-
You are correct. The only way to to "tag" style filtering is including keywords in the name. For example all "CI" tests could include the word "CI":
I would push back on the need for labels - why isn't the grouping and group-based filtering of google test sufficient? First, we should be very careful in introducing mechanisms that prevent certain tests from running - that is exactly how you end up with tests not being run in CI and regressions going unnoticed because someone forgets a flag somewhere. By default, all tests should run. For development, it is of course nice to only run the tests specific to the thing I am working on. But why can't I just pick the few groups of tests that are relevant? If the groups don't quite line up, google test can do quite some complex filtering: "Run all the tests in the suite (
At most, I can see a case for using tags to optionally exclude certain tests (again - not by default.). It is not uncommon to put "Slow" in the name of slow tests so I can do a @paolo-ienne @ebosnjak is there an application/use case for tags that I am missing? |
Beta Was this translation helpful? Give feedback.
-
No, not really. I just think that using superlong highly-structured names to make up for the absence of labels/tags is clumsy, but I can definitely survive.... 😉 |
Beta Was this translation helpful? Give feedback.
-
|
Thank you for your comments! I am glad to see that a great discussion came out of this proposal. First of all, I would say that the most important point given here is to stop reinventing the wheel and use an existing framework. This is my oversight since I have not had experience with such tools in the past. Hence, I cannot really comment on the concrete details of moving to such a framework (although some were mentioned here), but I understand at a high level that it is obviously a better solution. My main concern with the previous approach was the potential feature creep making the tool a nightmare to use. Also JSON configurations are a disaster waiting to happen (apparently it wouldn't be the first time, as @murphe67 mentioned). Another thing that worried me is that fact that @shundroid's testing requires a completely different compilation flow, i.e. overriding
However, I think that this other point given by @schilkp is much more critical:
I will put it this way: currently I have a Python script ( Finally, I would like to say something about "groups of tests", or whatever you want to call them. My problem is related to the following point by @schilkp:
So, if I am developing a feature and I make a set of tests for that purpose ("my tests"), it does indeed make sense that I would only want to run these tests (to save time while testing locally). Then, when I am satisfied, I would also need to make sure that all other (let's call them "regular") tests still pass. I believe that we all understand that this is the purpose of testing. And now when everything works and I merge my feature to main, I would say that it makes sense that "my tests" are added to the set of "regular" tests, since my feature is now a part of main. This is because these tests will need to be "regular" for everyone else, i.e. they will need to be run by everyone else developing other features, to make sure that their feature does not break my feature. For this reason, I do not understand the need to have many different groups of tests. I understand that this is a matter of policy and not capability, as @paolo-ienne said, but I find it hard to justify going out of our way to add capabilities that enable policies that defeat the purpose of (integration) testing. |
Beta Was this translation helpful? Give feedback.
-
Point taken, and if Google can live without them for Chromium and LLVM, I guess we can find a way to live without for Dynamatic.... |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
-
Hello everyone!
We recently introduced an Actions CI workflow and along with it, a Python script for running integration tests in a nicer way than before. However, this is not enough, because the new integration script only runs integration tests using "vanilla" Dynamatic, i.e. using only the default buffering algorithm when compiling.
This begs the question of how to make the script more flexible without making it complicated and tedious for use. For example, if I am developing some feature and (for whatever reason) I need to run a set of 10 tests with
--buffer-algorithm fpga20, I now need to run both the "official" set of tests (to make sure that I did not break anything) and my "custom" set of tests. So, not only does there need to be a way to include/exclude tests (that is better than the one we have right now with--listand--ignore), but it also needs to allow having various options for each test. However, this needs to be done in a way that would not ruin the usability of the integration system by making it horribly convoluted and requiring lots of manual intervention.Below I will give my suggestion, so please comment on it and propose your own if you have any. My absolute priority is usability; so I would be really happy if you suggested something that better minimizes the amount of manual labor required for using this system. So, I would like you, after reading this, to answer the question: Would you use this in your day-to-day work, or would you consider it tedious and write your own specific shell script instead?
Proposal
In order to allow running tests in with custom settings, there would be a way to define them in a configuration file. For example, having a file
integration-test/config.jsonwith the following content:{ "gcd": { "buffer-algorithm": "fpga20" }, "binary_search": { "timeout": 1000 }, "*": { "buffer-algorithm": "fpl22", "timeout": 200 }, "memory/*": { "timeout": 300 } }I hope that the meaning is self-explanatory; the point is that individual tests could be made to have custom properties and wildcards would be supported to avoid unnecessary repetition. This would also allow managing tests that are grouped in a subfolder, as shown in the last example. Also, at some point in the future, we plan to add a way to consistently benchmark tests in the Actions workflow (so you would get feedback that your PR ruined/improved the performance), so this would be easily extended to define benchmarking-related settings (e.g. "run test
binary_searchwith all buffering algorithms, but only compare performance using--buffer-algorithm fpga20"; this is just an example that I randomly thought about, please forgive me if it is nonsense).Such a configuration would allow the tests to be run very simply, the configuration file would be written and put in the
integration-testfolder and the script would read it when running and apply the correct settings. However, one concern is that this file may grow too large, and a proposed solution is to have configuration files per-test, for example, a fileintegration-test/gcd/config.jsonthat only contains settings for thegcdtest. On the other hand, my opinion is that opening different folders and editing different files is more tedious than scrolling through a single file, so I am not a big fan of this.Another goal would be to allow defining different sets of tests, in a more flexible way than the current run/ignore lists. For example, having a file
integration-test/sets.jsonwith the following contents:{ "ignore": ["gcd", "memory/test_memory_1"], "default": ["*", "!ignore"], "paolos_tests": ["kernel_3mm_float", "binary_search"] }With this, sets of tests could be run using:
python run_integration.pywould run thedefaultgrouppython run_integration.py paolos_testswould run thepaolos_testsgrouppython run_integration.py ignorewould run theignoregroup (i.e. the "ignored" tests can also be run as any other set).A problem that arises is how to reconcile the proposed
config.jsonandsets.json, since, for example, what if Paolo wanted to runbinary_searchwithtimeout=80, buffer-algorithm=fpga20when running the setpaolos_tests? One thing that comes to mind is to allow aliases to be defined for tests inconfig.json, e.g.{ "kernel_3mm_float-paolo": { "actual-path": "kernel_3mm_float", "timeout": 80, "buffer-algorithm": "fpga20" } }and then add
kernel_3mm_float-paoloto the list insets.json.Finally, there is a problem of the consistency of these configuration files between main and the current branch. If I change them because I want to run a bunch of tests, what happens when I want to merge? Do I have to just never commit the changes? This is also something that was brought up in a discussion with Paolo: with these integration tests, there should be a policy on what happens when someone adds new tests on their branch. In my opinion, if my feature required additional tests, it would make sense that these tests are added to the "default" set of tests and as such pushed to main. This is because these tests would make sure that no one breaks my feature later.
Conclusion
I understand that the implementation of these wildcards and whatever I mentioned above might be a bit tedious and not pretty, but this is just a suggestion that showcases its user experience, since that is my priority as I said before. So finally, I would like to ask you one more time to be very critical towards my idea and any other proposal that may be given by other people, since the goal is that this system could be used by even the laziest programmer, without him or her getting discouraged due to complexity. Honestly, being a lazy programmer myself, this seems like a bit too much manual labor, so I am looking forward to suggestions and improvements :)
Beta Was this translation helpful? Give feedback.
All reactions