Skip to content

Conversation

figueroa1395
Copy link
Member

@figueroa1395 figueroa1395 commented Sep 11, 2024

Re-route the validation tests through the C API:

  • Move the call for the tests to the C++ wrapper of the C API.
  • Remove the PGM core dependencies.

Re-route the benchmarks through the C API:
- [ ] Move the call for the benchmark to the C++ wrapper of the C API.
- [ ] Remove the PGM core dependencies.

Note: The re-routing of the benchmark won't be addressed here anymore, since it requieres more effort than initially anticipated. For the moment, it is taken out of the CI build and addressed on the nightly build #736

Closes #486

@TonyXiang8787
Copy link
Member

@figueroa1395 should the name of this PR be changed to only re-route validation tests?

@figueroa1395
Copy link
Member Author

@figueroa1395 should the name of this PR be changed to only re-route validation tests?

PR edited accordingly.

Signed-off-by: Santiago Figueroa Manrique <[email protected]>
@figueroa1395 figueroa1395 changed the title Feature/re route benchmark validation tests Feature/Re route validation tests Sep 26, 2024
@figueroa1395
Copy link
Member Author

Since I have started working on this PR again, this is very outdated at the moment. Many things have and will keep changing.

Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
@figueroa1395
Copy link
Member Author

At this stage all the single validation tests have been migrated to the Cpp API and pass. Next I will fix the batch validation.

Signed-off-by: Santiago Figueroa Manrique <[email protected]>
@TonyXiang8787
Copy link
Member

@figueroa1395 you need to resolve merge conflict, otherwise the CI will not start.

@figueroa1395
Copy link
Member Author

@TonyXiang8787 Thanks, I did not know. I am doing a bit of refactoring and once I finish that, I will solve the conflict.

Copy link
Member

@mgovers mgovers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great progress. will review the rest on monday when you're done cleaning up

@figueroa1395
Copy link
Member Author

The only things remaining at this point are doing a full manual clang tidy run and figuring out what is going on with the MacOS build.

Signed-off-by: Tony Xiang <[email protected]>
@TonyXiang8787
Copy link
Member

TonyXiang8787 commented Oct 5, 2024

The only things remaining at this point are doing a full manual clang tidy run and figuring out what is going on with the MacOS build.

@figueroa1395 @mgovers
After debugging in my Mac I have found the bug. The std::aligned_alloc has two requirements for unix-like system which we did not enforce:

  • Alignment should be multiple of sizeof(void*)
  • Size should be multiple of alignment

I have now pushed the fix.

@figueroa1395
Copy link
Member Author

@TonyXiang8787 Thanks! Great find and fix.

Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
@figueroa1395 figueroa1395 marked this pull request as ready for review October 7, 2024 06:26
Copy link
Member

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple minor comments.

Copy link
Member

@mgovers mgovers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

halfway through the review. continuing now but then you can already process what i have so far

Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Copy link

sonarqubecloud bot commented Oct 7, 2024

Copy link
Member

@TonyXiang8787 TonyXiang8787 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no open points and therefore approve the PR.

Feel free to put it in the merge queue.

@mgovers mgovers added this pull request to the merge queue Oct 7, 2024
Merged via the queue into main with commit dd0d1b0 Oct 7, 2024
28 checks passed
@mgovers mgovers deleted the feature/re-route-benchmark-validation-tests branch October 7, 2024 15:17
@mgovers mgovers mentioned this pull request Oct 8, 2024
2 tasks
@mgovers mgovers mentioned this pull request Nov 5, 2024
27 tasks
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.

[FEATURE] Optimize compilation speed by reorganize headers

4 participants