[docs] Added integration test instructions #268
Conversation
tools/integration/instructions.md
Outdated
| 1. The C source is compiled using Dynamatic | ||
| 2. The result is written to a HDL file | ||
| 3. The HDL description is simulated using ModelSim | ||
| 4. The test passes iff the simulation ends without errors |
There was a problem hiding this comment.
This is definitely misleading with respect to the hls_verify project, which uses the results from the C code to obtain the expected results from the kernel (in terms of returned value and content of the memories)
There was a problem hiding this comment.
I don't really know how this works under the hood, are you referring to the magic done by the CALL_KERNEL macro?
There was a problem hiding this comment.
I mean that the point number 4 is not true. Having an error-free simulation is a necessary condition for the kernel to work; then, once the simulation is done, the outputs/memory storages are compared to the expected values obtained by normally running the C file
There was a problem hiding this comment.
I updated this, let me know if the new text is better.
tools/integration/instructions.md
Outdated
|
|
||
| ## GitHub Actions workflow | ||
|
|
||
| In order to verify the compliance of contributions, an Actions workflow is set up for the purpose of integration testing. The workflow runs all integration tests (except the ones in `ignored_tests.txt`) when a pull request to the main branch is made. This does not apply to PRs marked as drafts, so if your PR is a draft, make sure to mark it as such to avoid running the workflow unnecessarily. Also, external contributors must receive approval to run the workflow on their PR due to security concerns. Please note that having the workflow successfully complete (i.e. pass all integration tests) is a necessary condition for having your PR approved. |
There was a problem hiding this comment.
Part of this line seems related to the file GettingStarted.md rather than in here.
There was a problem hiding this comment.
True, it might be better to add this into Getting Started and just put a link here.
There was a problem hiding this comment.
I moved this to Getting Started with the latest commit
| Integration tests are used to ensure proper functioning of all parts of Dynamatic. Unlike unit tests, which check the correctness of a single piece of the final software product (e.g. a single function/method), the purpose of integration tests is to make sure that Dynamatic as a whole yields correct outputs. In other words, that a C source file is compiled to a valid HDL description of a circuit that functions correctly (according to the original source file). | ||
|
|
There was a problem hiding this comment.
...yields correct outputs in a set of well-picked tests.
There was a problem hiding this comment.
What do you mean by this? Are you referring to the fact that we have excluded tests that don't work for known reasons? I mean, I just wanted to explain the concept of integration testing in one sentence here, so this isn't really important?
There was a problem hiding this comment.
True, maybe I was just too picky hahahahaha
|
Hello, I am currently having an issue with PR #278. I have just changed a Verilog file, and the CI/CD flow fails for no reason. Apparently, it's also trying to link clang-18 for no reason. |
|
Maybe we should do -f in build.sh? |
|
@Carmine50 I looked at the log, and it failed because Polygeist compilation crashed due to running out of memory. This is an issue that happens sometimes and AFAIK there isn't really a fix for it, so I scheduled your workflow for another run. I hope it works the second time, if not, I'll make it run in single-threaded mode, which will be slow but will have to work. |
|
Another workaround is to increase the swap memory so it can link clang |
|
Yes, it worked thank you :) Is there no automatic way to dynamically increase memory size if polygeist crashes? |
|
@Jiahui17 @Carmine50 I'll try increasing the amount of swap, but I'm not sure that would solve it. The memory doesn't seem to be the only issue, since I get crashes locally even with 32 GB of RAM that does not fill up all the way. The good thing is that the runner does not delete build files, so it is only necessary to compile Polygeist once and after that it will be skipped by the build system. For some reason, that was not the case yesterday when a recompilation was triggered and caused a crash in your workflow. |
|
@ebosnjak So the goal would be avoiding recompiling polygeist as much as possible. So potentially we would rarely have this issue since the majority of the time we do not modify polygeist. |
|
@Carmine50 Exactly, this should not happen often. Even if it does, just restart the workflow if you see the build error happen. I'll look into this issue and try to find a way to fix it. |
This PR adds a .md file with general information regarding Dynamatic integration tests and instructions on how to use the Python integration test script.