-
Notifications
You must be signed in to change notification settings - Fork 8
check rcppdeepstate from this repo #13
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
base: master
Are you sure you want to change the base?
Conversation
|
hey @FabrizioSandri it says "No error has been reported by RcppDeepState" but I'm wondering what functions were tested, and with how many random inputs. Can that info be added to the comment? |
|
also i'm thinking that we can add a third option, comment: 'failure' means only comment when there is a failure. (probably when there are no issues, most users will like to see nothing / not be bothered) |
|
|
@FabrizioSandri Are you running valgrind with --leak-check=full? If not then you don't get the details about what line of code caused the leak. |
Yes, this is a good improvement. I will add this |
After digging inside the code I found that if there is at least one function that cannot be analyzed, that is, a function whose arguments differ from the supported one, RcppDeepState terminates. I solved this problem in the pull request Fuzz only supported functions #12. I am waiting for the CI checks and then I will merge that pull request so that we can check again if RcppDeepState-action works on the binsegRcpp package. |
The leak check is automatically set to
|
|
is it the LogicalVector which was not supported? |
|
Yes, exactly |
These new features has been implemented in the pull request FabrizioSandri/RcppDeepState-action#6. Now I focus on the memory leak that has not been reported. |
|
I re-ran the job, https://github.com/tdhock/binsegRcpp/actions/runs/2741524510 |
|
@tdhock this is a great new feature, I've just implemented and tested it in FabrizioSandri/RcppDeepState-action#6. |
|
After re-running the job https://github.com/tdhock/binsegRcpp/runs/7564724127?check_suite_focus=true I don't think it is working. I expected to see some report on a PR comment or at least on the github action log, but I do not see anything. Can you please investigate/fix? |
|
Yes, the problem is that the changes I made are on the |
|
Ok, I have just merged the pull request branch into |
RcppDeepState Report
Analyzed functions summary
Report details
|
|
From the last comment It seems like the inputs are too big so the cell content is overflowing, I'll investigate |
|
thanks! |
|
also I tried following the hyperlink https://github.com/github.com/tdhock/binsegRcpp/blob/check-rcppdeepstate/src/interface.cpp#L11 which says not found. |
|
also is there documentation about how I can write a custom test harness for my binseg_interface function? I would like to be able to manually specify some C++ code for how to do random generation of that LogicalVector and the other inputs. |
Thanks @tdhock, I followed your suggestion and added the paste command with the collapse parameter. I also removed the Here is a description of the changes I made to solve these two issues: FabrizioSandri/RcppDeepState-action#7 (comment) |
I solved this problem, a more detailed description can be found in FabrizioSandri/RcppDeepState-action#7 (comment) |
I start writing a blog post outlining the procedure for manually creating a test harness and analyzing it using RcppDeepState. |
| return rand_vec; | ||
| } | ||
|
|
||
| #define INPUTS \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could use a macro like this to avoid duplication in definition of data /generator code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tdhock I have revised the harness creation procedure of RcppDeepState and made some additional improvements, including the use of macros. You can find these new changes in FabrizioSandri/RcppDeepState#16.
Before testing this change I have to merge FabrizioSandri/RcppDeepState#16 into the main branch. I will wait for your code review before merging these modifications, so that if something is wrong I can simply adjust.
|
seems like the most recent run failed https://github.com/tdhock/binsegRcpp/runs/7680915937?check_suite_focus=true Do you know why @FabrizioSandri ? |
|
@tdhock The issue was that RcppDeepState deletes the existing Now we can run CI again, and check if |
|
I have completed the action integration with Docker Hub with pull request FabrizioSandri/RcppDeepState-action#9. I ran the docker action on
I added added the summary table in the comment with pull request FabrizioSandri/RcppDeepState-action#9 |
|
After re-running https://github.com/tdhock/binsegRcpp/runs/7718406649?check_suite_focus=true it looks like it saw the manual test harness but did not either compile or execute it, can you please investigate? |
Hi @tdhock I have been working on this and found that the problem was due to RcppDeepState that was looking for the I solved this problem with pull request FabrizioSandri/RcppDeepState#19. |
|
@tdhock I wrote a blog post last week about how to Write a custom test harness for functions with unsupported datatypes. |
|
hi again @FabrizioSandri I re-ran the build and it seems to be passing now, but it should not be, because there is still a memory leak. What is the problem? |
|
@thock I've tried to manually run the same code inside R with Valgrind as a debugger as you did in your previous comment, and I don't see any errors. On my system, I ran Previously, the Action was reporting the error, so I assumed that the issue was due to the non-deterministic nature of the inputs. However I have been able to prove that this is not the motivation: I have tried to reproduce the same inputs that generated the error in the previous runs, by downloading the relative artifact file and locating he inputs that were used( Again, no error has been reported, indicating that the error is not related to the inputs. So I'm thinking that this issue is caused by the system configuration or the version of clang used, or maybe by the compiler parameters used when installing the package. I'm overlooking something. Could you please tell me the Clang version you used to compile and install the binsegRcpp package? |
|
you are right: the leak should happen for any inputs. Did you try |
|
@tdhock, I just realized that the problem is that I cloned the I also fixed the issue where no error was reported; it was caused by the harness test name. Pull request FabrizioSandri/RcppDeepState#21 has been submitted to fix the issue. Now we can run CI again, to check if the error is solved. |
|
@FabrizioSandri do you have any idea why RcppDeepState action was not run on the last push to this PR? |
|
I guess it was because there was a tab instead of space for indent in the yaml, sorry for the trouble. |
.github/workflows/RcppDeepState.yaml
Outdated
| comment: 'true' | ||
| comment: 'true' | ||
| max_inputs: 10 | ||
| seed: 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RcppDeepState-action has not been executed because of this wrong indentation of the workflow file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tdhock I sent you this review yesterday and noticed the pending label. I assumed that 'Pending' meant "Your comment is awaiting review by the maintainer". Instead, it means that I have yet to submit my review; there is an additional submit button before submitting the review.

No description provided.