ARROW-12571: [R][CI] Run nightly R with valgrind - #10237

Closed
jonkeane wants to merge 4 commits into
apache:masterfrom
jonkeane:ARROW-12571-valgrindCI-patch
Closed

ARROW-12571: [R][CI] Run nightly R with valgrind#10237
jonkeane wants to merge 4 commits into
apache:masterfrom
jonkeane:ARROW-12571-valgrindCI-patch

Conversation

@jonkeane

@jonkeanejonkeane commented May 3, 2021

Copy link
Copy Markdown
Member

No description provided.

@github-actions

github-actionsBot commented May 3, 2021

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-12571

@jonkeane

jonkeane commented May 3, 2021

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit test-r-linux-valgrind

@github-actions

github-actionsBot commented May 3, 2021

Copy link
Copy Markdown

Revision: 0e1d0e219c3e05b52ad180bd626a499138b577d6

Submitted crossbow builds: ursacomputing/crossbow @ actions-377

TaskStatus
test-r-linux-valgrindAzure

pitrou
pitrou reviewed May 4, 2021
View reviewed changes
Comment threadci/scripts/r_valgrind.sh Outdated

@pitroupitrouMay 4, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You should use valgrind --error-exitcode=1 instead of grepping.

Comment threadci/scripts/r_valgrind.sh Outdated

@pitroupitrouMay 4, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should use a Valgrind suppressions file instead of grepping:
https://stackoverflow.com/questions/17159578/generating-suppressions-for-memory-leaks

Comment threadr/tests/testthat/test-compute-aggregate.R Outdated

@pitroupitrouMay 4, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why these changes? If they have a specific purpose, there should be a comment explaining them.

pitrou
pitrou reviewed May 4, 2021
View reviewed changes
Comment threadci/scripts/r_valgrind.sh Outdated

@pitroupitrouMay 4, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't this dangerous if there are CI secrets in environment variables? Why is this needed?

@jonkeanejonkeaneMay 4, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I was using it to diagnose if the ARROW_RUNTIME_SIMD_LEVEL var was being passed — secrets should be redacted even with something like printenv, but I've removed it regardless.

@pitrou

pitrou commented May 4, 2021

Copy link
Copy Markdown
Member

I'm trying this out and I see that it takes a lot of time without outputting anything:

+ RDvalgrind --vanilla -d 'valgrind --tool=memcheck --leak-check=full --track-origins=yes' -f testthat.R

Can the above command output progress while running the test suite?

@jonkeane

jonkeane commented May 4, 2021

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit test-r-linux-valgrind

@jonkeanejonkeane closed this May 4, 2021
@jonkeanejonkeane reopened this May 4, 2021
@jonkeane

jonkeane commented May 4, 2021

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit test-r-linux-valgrind

@github-actions

github-actionsBot commented May 4, 2021

Copy link
Copy Markdown

Revision: 041f4095fb71adfbfb5d504ff83da04c07cb4352

Submitted crossbow builds: ursacomputing/crossbow @ actions-379

TaskStatus
test-r-linux-valgrindAzure

@github-actions

github-actionsBot commented May 4, 2021

Copy link
Copy Markdown

Revision: 041f4095fb71adfbfb5d504ff83da04c07cb4352

Submitted crossbow builds: ursacomputing/crossbow @ actions-380

TaskStatus
test-r-linux-valgrindAzure

@jonkeane

jonkeane commented May 4, 2021

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit test-r-linux-valgrind

@github-actions

github-actionsBot commented May 4, 2021

Copy link
Copy Markdown

Revision: 4ede120d38854d6bbf7556450ed15e8345b59fcd

Submitted crossbow builds: ursacomputing/crossbow @ actions-381

TaskStatus
test-r-linux-valgrindAzure

nealrichardson
nealrichardson reviewed May 4, 2021
View reviewed changes
Comment threadr/tests/testthat/test-compute-aggregate.R Outdated
Comment on lines 40 to 41

@nealrichardsonnealrichardsonMay 4, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could also do something like (of course confirming the right way to determine a devel version). Could also add the devel check to the skip_on_linux_cran (and call it skip_on_valgrind()) if you wanted to make that narrower.

Suggested change
# expect_identical(as.numeric(sum(na)), sum(floats))
expect_identical(as.numeric(sum(na)), NA_real_)
if (!grepl("devel", R.version.string)) {
# Valgrind on R-devel confuses NaN and NA_real_
expect_identical(as.numeric(sum(na)), sum(floats))
}

@jonkeanejonkeaneMay 4, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yeah, I'll add the devel check to skip_on_linux_cran() (and probably rename it too, though I'm a bit hesitant to make it so obvious that we are skipping the problematic tests — is that something we should worry about?).

As for these specific tests, looking at it now I wonder if we have benefit from expect_identical(as.numeric(sum(na)), sum(floats)) versus expect_identical(as.numeric(sum(na)), NA_real_)? Is there a circumstance where we expect sum(floats) to change behavior and we would want to match it? I find expect_identical(as.numeric(sum(na)), NA_real_) to be easier to read at a glance and see exactly what it's testing/expecting.

nealrichardson
nealrichardson reviewed May 4, 2021
View reviewed changes
Comment threaddocker-compose.yml Outdated
@jonkeane

jonkeane commented May 4, 2021

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit test-r-linux-valgrind

@github-actions

github-actionsBot commented May 4, 2021

Copy link
Copy Markdown

Revision: 7f29abc988154513ad8f43a789b3399c3dfd971c

Submitted crossbow builds: ursacomputing/crossbow @ actions-384

TaskStatus
test-r-linux-valgrindAzure

@jonkeane

jonkeane commented May 4, 2021

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit test-r-linux-valgrind

(with mimalloc as default)

@jonkeane

jonkeane commented May 4, 2021

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit test-r-linux-valgrind

@github-actions

github-actionsBot commented May 4, 2021

Copy link
Copy Markdown

Revision: a94cff3c451e708e387d27dd89e9832b2eedbfea

Submitted crossbow builds: ursacomputing/crossbow @ actions-385

TaskStatus
test-r-linux-valgrindAzure

nealrichardson
nealrichardson requested changes May 4, 2021
View reviewed changes

@nealrichardsonnealrichardson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks a lot for taking this on.

Comment threadr/tests/testthat/helper-skip.R Outdated
Comment threadr/tests/testthat/helper-skip.R Outdated
Comment threaddocker-compose.yml Outdated
Comment on lines 1072 to 1073

@nealrichardsonnealrichardsonMay 4, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
ARROW_USER_SIMD_LEVEL: "AVX2"
ARROW_RUNTIME_SIMD_LEVEL: "AVX2"

Comment threadci/scripts/r_valgrind.sh Outdated

@nealrichardsonnealrichardsonMay 4, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Have you confirmed that this job (in the script's final form) correctly fails if there's a valgrind failure? You could try this by making skip_on_valgrind() no-op and see if it fails as expected, then restore the skips.

@jonkeanejonkeaneMay 4, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes it does, though I will re-confirm that once we are close to merging

@jonkeane
jonkeaneforce-pushed the ARROW-12571-valgrindCI-patch branch from 7886dcb to 5a7493eCompareMay 4, 2021 18:00
@jonkeane
jonkeane changed the base branch from release-4.0.0 to masterMay 4, 2021 18:11
@jonkeane
jonkeaneforce-pushed the ARROW-12571-valgrindCI-patch branch from 5a7493e to 3ab8871CompareMay 4, 2021 18:11
@jonkeane

jonkeane commented May 4, 2021

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit test-r-linux-valgrind

@github-actions

github-actionsBot commented May 4, 2021

Copy link
Copy Markdown

Revision: 3ab8871

Submitted crossbow builds: ursacomputing/crossbow @ actions-389

TaskStatus
test-r-linux-valgrindAzure

@jonkeane

jonkeane commented May 4, 2021

Copy link
Copy Markdown
MemberAuthor

If ^^^ doesn't pass, there was a merge/squash error

@jonkeane

jonkeane commented May 4, 2021

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit test-r-linux-valgrind

@jonkeane

jonkeane commented May 4, 2021

Copy link
Copy Markdown
MemberAuthor

We don't expect ^^^ to pass, it is intentionally broken

@github-actions

github-actionsBot commented May 4, 2021

Copy link
Copy Markdown

Revision: 324786f

Submitted crossbow builds: ursacomputing/crossbow @ actions-390

TaskStatus
test-r-linux-valgrindAzure

@jonkeane
jonkeaneforce-pushed the ARROW-12571-valgrindCI-patch branch from e1a8bd3 to 3bb94e6CompareMay 4, 2021 21:18
@jonkeane

jonkeane commented May 4, 2021

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit test-r-linux-valgrind

@github-actions

github-actionsBot commented May 4, 2021

Copy link
Copy Markdown

Revision: 3bb94e6

Submitted crossbow builds: ursacomputing/crossbow @ actions-391

TaskStatus
test-r-linux-valgrindAzure

@jonkeane

jonkeane commented May 4, 2021

Copy link
Copy Markdown
MemberAuthor

We don't expect ^^^ to pass, it is intentionally broken

@jonkeane

jonkeane commented May 4, 2021

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit test-r-linux-valgrind

@jonkeane

jonkeane commented May 4, 2021

Copy link
Copy Markdown
MemberAuthor

And ^^^ should pass now that we reverted our intentional failures.

@github-actions

github-actionsBot commented May 4, 2021

Copy link
Copy Markdown

Revision: 5812a19

Submitted crossbow builds: ursacomputing/crossbow @ actions-392

TaskStatus
test-r-linux-valgrindAzure

@jonkeane
jonkeane deleted the ARROW-12571-valgrindCI-patch branch May 5, 2021 12:51
jonkeane added a commit to jonkeane/arrow that referenced this pull request May 5, 2021
@jonkeane
ARROW-12571: [R][CI] Run nightly R with valgrind
1ced508
Closesapache#10237 from jonkeane/ARROW-12571-valgrindCI-patch
Authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
kszucs pushed a commit to kszucs/arrow that referenced this pull request May 17, 2021
@jonkeane@kszucs
ARROW-12571: [R][CI] Run nightly R with valgrind
be5f704
Closesapache#10237 from jonkeane/ARROW-12571-valgrindCI-patch
Authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
@asfimportasfimport mentioned this pull request May 5, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pitroupitroupitrou left review comments
@nealrichardsonnealrichardsonnealrichardson approved these changes

Assignees

No one assigned

Labels

Component: R

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jonkeane@pitrou@nealrichardson
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
ARROW-12571: [R][CI] Run nightly R with valgrind - #10237
Closed
jonkeane wants to merge 4 commits into
apache:masterfrom 
jonkeane:ARROW-12571-valgrindCI-patch
Closed
ARROW-12571: [R][CI] Run nightly R with valgrind#10237
jonkeane wants to merge 4 commits into
apache:masterfrom 
jonkeane:ARROW-12571-valgrindCI-patch
Conversation
@jonkeane
@jonkeanejonkeane
commented
May 3, 2021

Copy link


Copy Markdown

Member
 No description provided. 
@github-actions
github-actionsBot
commented
May 3, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-12571
@jonkeane
jonkeane
commented
May 3, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 3, 2021

Copy link


Copy Markdown

Revision: 0e1d0e219c3e05b52ad180bd626a499138b577d6
Submitted crossbow builds: ursacomputing/crossbow @ actions-377
TaskStatus
test-r-linux-valgrindAzure
pitrou
pitrou
reviewed
May 4, 2021
View reviewed changes
Comment threadci/scripts/r_valgrind.sh
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

You should use valgrind --error-exitcode=1 instead of grepping.
Comment threadci/scripts/r_valgrind.sh
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We should use a Valgrind suppressions file instead of grepping:
https://stackoverflow.com/questions/17159578/generating-suppressions-for-memory-leaks
Comment threadr/tests/testthat/test-compute-aggregate.R
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Why these changes? If they have a specific purpose, there should be a comment explaining them.
pitrou
pitrou
reviewed
May 4, 2021
View reviewed changes
Comment threadci/scripts/r_valgrind.sh
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Isn't this dangerous if there are CI secrets in environment variables? Why is this needed?
@jonkeanejonkeaneMay 4, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I was using it to diagnose if the ARROW_RUNTIME_SIMD_LEVEL var was being passed — secrets should be redacted even with something like printenv, but I've removed it regardless.
@pitrou
pitrou
commented
May 4, 2021

Copy link


Copy Markdown

Member
I'm trying this out and I see that it takes a lot of time without outputting anything:
+ RDvalgrind --vanilla -d 'valgrind --tool=memcheck --leak-check=full --track-origins=yes' -f testthat.R

Can the above command output progress while running the test suite?
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@jonkeanejonkeane
closed this
May 4, 2021
@jonkeanejonkeane
reopened this
May 4, 2021
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 041f4095fb71adfbfb5d504ff83da04c07cb4352
Submitted crossbow builds: ursacomputing/crossbow @ actions-379
TaskStatus
test-r-linux-valgrindAzure
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 041f4095fb71adfbfb5d504ff83da04c07cb4352
Submitted crossbow builds: ursacomputing/crossbow @ actions-380
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 4ede120d38854d6bbf7556450ed15e8345b59fcd
Submitted crossbow builds: ursacomputing/crossbow @ actions-381
TaskStatus
test-r-linux-valgrindAzure
nealrichardson
nealrichardson
reviewed
May 4, 2021
View reviewed changes
Comment threadr/tests/testthat/test-compute-aggregate.R
Outdated


Comment on lines

40
 to 
41

@nealrichardsonnealrichardsonMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Could also do something like (of course confirming the right way to determine a devel version). Could also add the devel check to the skip_on_linux_cran (and call it skip_on_valgrind()) if you wanted to make that narrower.

Suggested change

# expect_identical(as.numeric(sum(na)), sum(floats))
 expect_identical(as.numeric(sum(na)), NA_real_)
if (!grepl("devel", R.version.string)) {
# Valgrind on R-devel confuses NaN and NA_real_
 expect_identical(as.numeric(sum(na)), sum(floats))
 }
@jonkeanejonkeaneMay 4, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yeah, I'll add the devel check to skip_on_linux_cran() (and probably rename it too, though I'm a bit hesitant to make it so obvious that we are skipping the problematic tests — is that something we should worry about?).
As for these specific tests, looking at it now I wonder if we have benefit from expect_identical(as.numeric(sum(na)), sum(floats)) versus expect_identical(as.numeric(sum(na)), NA_real_)? Is there a circumstance where we expect sum(floats) to change behavior and we would want to match it? I find expect_identical(as.numeric(sum(na)), NA_real_) to be easier to read at a glance and see exactly what it's testing/expecting.
nealrichardson
nealrichardson
reviewed
May 4, 2021
View reviewed changes
Comment threaddocker-compose.yml
Outdated

@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 7f29abc988154513ad8f43a789b3399c3dfd971c
Submitted crossbow builds: ursacomputing/crossbow @ actions-384
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
(with mimalloc as default)
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: a94cff3c451e708e387d27dd89e9832b2eedbfea
Submitted crossbow builds: ursacomputing/crossbow @ actions-385
TaskStatus
test-r-linux-valgrindAzure
nealrichardson
nealrichardson
requested changes
May 4, 2021
View reviewed changes
@nealrichardsonnealrichardson
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Thanks a lot for taking this on.
Comment threadr/tests/testthat/helper-skip.R
Outdated

Comment threadr/tests/testthat/helper-skip.R
Outdated

Comment threaddocker-compose.yml
Outdated


Comment on lines

1072
 to 
1073

@nealrichardsonnealrichardsonMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

ARROW_USER_SIMD_LEVEL: "AVX2"
ARROW_RUNTIME_SIMD_LEVEL: "AVX2"
Comment threadci/scripts/r_valgrind.sh
Outdated

@nealrichardsonnealrichardsonMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Have you confirmed that this job (in the script's final form) correctly fails if there's a valgrind failure? You could try this by making skip_on_valgrind() no-op and see if it fails as expected, then restore the skips.
@jonkeanejonkeaneMay 4, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes it does, though I will re-confirm that once we are close to merging
@jonkeane
jonkeaneforce-pushed
the
ARROW-12571-valgrindCI-patch
branch
from
7886dcb to
5a7493eCompareMay 4, 2021 18:00
@jonkeane
jonkeane
changed the base branch from
release-4.0.0
to
masterMay 4, 2021 18:11
@jonkeane
jonkeaneforce-pushed
the
ARROW-12571-valgrindCI-patch
branch
from
5a7493e to
3ab8871CompareMay 4, 2021 18:11
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 3ab8871
Submitted crossbow builds: ursacomputing/crossbow @ actions-389
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
If ^^^ doesn't pass, there was a merge/squash error
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
We don't expect ^^^ to pass, it is intentionally broken
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 324786f
Submitted crossbow builds: ursacomputing/crossbow @ actions-390
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeaneforce-pushed
the
ARROW-12571-valgrindCI-patch
branch
from
e1a8bd3 to
3bb94e6CompareMay 4, 2021 21:18
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 3bb94e6
Submitted crossbow builds: ursacomputing/crossbow @ actions-391
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
We don't expect ^^^ to pass, it is intentionally broken
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
And ^^^ should pass now that we reverted our intentional failures.
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 5812a19
Submitted crossbow builds: ursacomputing/crossbow @ actions-392
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
deleted the

ARROW-12571-valgrindCI-patch

branch
May 5, 2021 12:51
jonkeane added a commit
to jonkeane/arrow
that referenced
this pull request
May 5, 2021
@jonkeane
ARROW-12571: [R][CI] Run nightly R with valgrind
1ced508
Closesapache#10237 from jonkeane/ARROW-12571-valgrindCI-patch
Authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
kszucs pushed a commit
to kszucs/arrow
that referenced
this pull request
May 17, 2021
@jonkeane@kszucs
ARROW-12571: [R][CI] Run nightly R with valgrind
be5f704
Closesapache#10237 from jonkeane/ARROW-12571-valgrindCI-patch
Authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
@asfimportasfimport
mentioned this pull request
May 5, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@pitroupitroupitrou left review comments
@nealrichardsonnealrichardsonnealrichardson approved these changes

Assignees


No one assigned


Labels

Component: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@jonkeane@pitrou@nealrichardson

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-12571: [R][CI] Run nightly R with valgrind - #10237
Closed
jonkeane wants to merge 4 commits into
apache:masterfrom 
jonkeane:ARROW-12571-valgrindCI-patch
Closed
ARROW-12571: [R][CI] Run nightly R with valgrind#10237
jonkeane wants to merge 4 commits into
apache:masterfrom 
jonkeane:ARROW-12571-valgrindCI-patch
Conversation
@jonkeane
@jonkeanejonkeane
commented
May 3, 2021

Copy link


Copy Markdown

Member
 No description provided. 
@github-actions
github-actionsBot
commented
May 3, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-12571
@jonkeane
jonkeane
commented
May 3, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 3, 2021

Copy link


Copy Markdown

Revision: 0e1d0e219c3e05b52ad180bd626a499138b577d6
Submitted crossbow builds: ursacomputing/crossbow @ actions-377
TaskStatus
test-r-linux-valgrindAzure
pitrou
pitrou
reviewed
May 4, 2021
View reviewed changes
Comment threadci/scripts/r_valgrind.sh
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

You should use valgrind --error-exitcode=1 instead of grepping.
Comment threadci/scripts/r_valgrind.sh
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We should use a Valgrind suppressions file instead of grepping:
https://stackoverflow.com/questions/17159578/generating-suppressions-for-memory-leaks
Comment threadr/tests/testthat/test-compute-aggregate.R
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Why these changes? If they have a specific purpose, there should be a comment explaining them.
pitrou
pitrou
reviewed
May 4, 2021
View reviewed changes
Comment threadci/scripts/r_valgrind.sh
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Isn't this dangerous if there are CI secrets in environment variables? Why is this needed?
@jonkeanejonkeaneMay 4, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I was using it to diagnose if the ARROW_RUNTIME_SIMD_LEVEL var was being passed — secrets should be redacted even with something like printenv, but I've removed it regardless.
@pitrou
pitrou
commented
May 4, 2021

Copy link


Copy Markdown

Member
I'm trying this out and I see that it takes a lot of time without outputting anything:
+ RDvalgrind --vanilla -d 'valgrind --tool=memcheck --leak-check=full --track-origins=yes' -f testthat.R

Can the above command output progress while running the test suite?
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@jonkeanejonkeane
closed this
May 4, 2021
@jonkeanejonkeane
reopened this
May 4, 2021
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 041f4095fb71adfbfb5d504ff83da04c07cb4352
Submitted crossbow builds: ursacomputing/crossbow @ actions-379
TaskStatus
test-r-linux-valgrindAzure
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 041f4095fb71adfbfb5d504ff83da04c07cb4352
Submitted crossbow builds: ursacomputing/crossbow @ actions-380
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 4ede120d38854d6bbf7556450ed15e8345b59fcd
Submitted crossbow builds: ursacomputing/crossbow @ actions-381
TaskStatus
test-r-linux-valgrindAzure
nealrichardson
nealrichardson
reviewed
May 4, 2021
View reviewed changes
Comment threadr/tests/testthat/test-compute-aggregate.R
Outdated


Comment on lines

40
 to 
41

@nealrichardsonnealrichardsonMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Could also do something like (of course confirming the right way to determine a devel version). Could also add the devel check to the skip_on_linux_cran (and call it skip_on_valgrind()) if you wanted to make that narrower.

Suggested change

# expect_identical(as.numeric(sum(na)), sum(floats))
 expect_identical(as.numeric(sum(na)), NA_real_)
if (!grepl("devel", R.version.string)) {
# Valgrind on R-devel confuses NaN and NA_real_
 expect_identical(as.numeric(sum(na)), sum(floats))
 }
@jonkeanejonkeaneMay 4, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yeah, I'll add the devel check to skip_on_linux_cran() (and probably rename it too, though I'm a bit hesitant to make it so obvious that we are skipping the problematic tests — is that something we should worry about?).
As for these specific tests, looking at it now I wonder if we have benefit from expect_identical(as.numeric(sum(na)), sum(floats)) versus expect_identical(as.numeric(sum(na)), NA_real_)? Is there a circumstance where we expect sum(floats) to change behavior and we would want to match it? I find expect_identical(as.numeric(sum(na)), NA_real_) to be easier to read at a glance and see exactly what it's testing/expecting.
nealrichardson
nealrichardson
reviewed
May 4, 2021
View reviewed changes
Comment threaddocker-compose.yml
Outdated

@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 7f29abc988154513ad8f43a789b3399c3dfd971c
Submitted crossbow builds: ursacomputing/crossbow @ actions-384
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
(with mimalloc as default)
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: a94cff3c451e708e387d27dd89e9832b2eedbfea
Submitted crossbow builds: ursacomputing/crossbow @ actions-385
TaskStatus
test-r-linux-valgrindAzure
nealrichardson
nealrichardson
requested changes
May 4, 2021
View reviewed changes
@nealrichardsonnealrichardson
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Thanks a lot for taking this on.
Comment threadr/tests/testthat/helper-skip.R
Outdated

Comment threadr/tests/testthat/helper-skip.R
Outdated

Comment threaddocker-compose.yml
Outdated


Comment on lines

1072
 to 
1073

@nealrichardsonnealrichardsonMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

ARROW_USER_SIMD_LEVEL: "AVX2"
ARROW_RUNTIME_SIMD_LEVEL: "AVX2"
Comment threadci/scripts/r_valgrind.sh
Outdated

@nealrichardsonnealrichardsonMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Have you confirmed that this job (in the script's final form) correctly fails if there's a valgrind failure? You could try this by making skip_on_valgrind() no-op and see if it fails as expected, then restore the skips.
@jonkeanejonkeaneMay 4, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes it does, though I will re-confirm that once we are close to merging
@jonkeane
jonkeaneforce-pushed
the
ARROW-12571-valgrindCI-patch
branch
from
7886dcb to
5a7493eCompareMay 4, 2021 18:00
@jonkeane
jonkeane
changed the base branch from
release-4.0.0
to
masterMay 4, 2021 18:11
@jonkeane
jonkeaneforce-pushed
the
ARROW-12571-valgrindCI-patch
branch
from
5a7493e to
3ab8871CompareMay 4, 2021 18:11
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 3ab8871
Submitted crossbow builds: ursacomputing/crossbow @ actions-389
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
If ^^^ doesn't pass, there was a merge/squash error
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
We don't expect ^^^ to pass, it is intentionally broken
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 324786f
Submitted crossbow builds: ursacomputing/crossbow @ actions-390
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeaneforce-pushed
the
ARROW-12571-valgrindCI-patch
branch
from
e1a8bd3 to
3bb94e6CompareMay 4, 2021 21:18
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 3bb94e6
Submitted crossbow builds: ursacomputing/crossbow @ actions-391
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
We don't expect ^^^ to pass, it is intentionally broken
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
And ^^^ should pass now that we reverted our intentional failures.
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 5812a19
Submitted crossbow builds: ursacomputing/crossbow @ actions-392
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
deleted the

ARROW-12571-valgrindCI-patch

branch
May 5, 2021 12:51
jonkeane added a commit
to jonkeane/arrow
that referenced
this pull request
May 5, 2021
@jonkeane
ARROW-12571: [R][CI] Run nightly R with valgrind
1ced508
Closesapache#10237 from jonkeane/ARROW-12571-valgrindCI-patch
Authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
kszucs pushed a commit
to kszucs/arrow
that referenced
this pull request
May 17, 2021
@jonkeane@kszucs
ARROW-12571: [R][CI] Run nightly R with valgrind
be5f704
Closesapache#10237 from jonkeane/ARROW-12571-valgrindCI-patch
Authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
@asfimportasfimport
mentioned this pull request
May 5, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@pitroupitroupitrou left review comments
@nealrichardsonnealrichardsonnealrichardson approved these changes

Assignees


No one assigned


Labels

Component: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@jonkeane@pitrou@nealrichardson

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-12571: [R][CI] Run nightly R with valgrind - #10237
Closed
jonkeane wants to merge 4 commits into
apache:masterfrom 
jonkeane:ARROW-12571-valgrindCI-patch
Closed
ARROW-12571: [R][CI] Run nightly R with valgrind#10237
jonkeane wants to merge 4 commits into
apache:masterfrom 
jonkeane:ARROW-12571-valgrindCI-patch
Conversation
@jonkeane
@jonkeanejonkeane
commented
May 3, 2021

Copy link


Copy Markdown

Member
 No description provided. 
@github-actions
github-actionsBot
commented
May 3, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-12571
@jonkeane
jonkeane
commented
May 3, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 3, 2021

Copy link


Copy Markdown

Revision: 0e1d0e219c3e05b52ad180bd626a499138b577d6
Submitted crossbow builds: ursacomputing/crossbow @ actions-377
TaskStatus
test-r-linux-valgrindAzure
pitrou
pitrou
reviewed
May 4, 2021
View reviewed changes
Comment threadci/scripts/r_valgrind.sh
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

You should use valgrind --error-exitcode=1 instead of grepping.
Comment threadci/scripts/r_valgrind.sh
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We should use a Valgrind suppressions file instead of grepping:
https://stackoverflow.com/questions/17159578/generating-suppressions-for-memory-leaks
Comment threadr/tests/testthat/test-compute-aggregate.R
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Why these changes? If they have a specific purpose, there should be a comment explaining them.
pitrou
pitrou
reviewed
May 4, 2021
View reviewed changes
Comment threadci/scripts/r_valgrind.sh
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Isn't this dangerous if there are CI secrets in environment variables? Why is this needed?
@jonkeanejonkeaneMay 4, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I was using it to diagnose if the ARROW_RUNTIME_SIMD_LEVEL var was being passed — secrets should be redacted even with something like printenv, but I've removed it regardless.
@pitrou
pitrou
commented
May 4, 2021

Copy link


Copy Markdown

Member
I'm trying this out and I see that it takes a lot of time without outputting anything:
+ RDvalgrind --vanilla -d 'valgrind --tool=memcheck --leak-check=full --track-origins=yes' -f testthat.R

Can the above command output progress while running the test suite?
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@jonkeanejonkeane
closed this
May 4, 2021
@jonkeanejonkeane
reopened this
May 4, 2021
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 041f4095fb71adfbfb5d504ff83da04c07cb4352
Submitted crossbow builds: ursacomputing/crossbow @ actions-379
TaskStatus
test-r-linux-valgrindAzure
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 041f4095fb71adfbfb5d504ff83da04c07cb4352
Submitted crossbow builds: ursacomputing/crossbow @ actions-380
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 4ede120d38854d6bbf7556450ed15e8345b59fcd
Submitted crossbow builds: ursacomputing/crossbow @ actions-381
TaskStatus
test-r-linux-valgrindAzure
nealrichardson
nealrichardson
reviewed
May 4, 2021
View reviewed changes
Comment threadr/tests/testthat/test-compute-aggregate.R
Outdated


Comment on lines

40
 to 
41

@nealrichardsonnealrichardsonMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Could also do something like (of course confirming the right way to determine a devel version). Could also add the devel check to the skip_on_linux_cran (and call it skip_on_valgrind()) if you wanted to make that narrower.

Suggested change

# expect_identical(as.numeric(sum(na)), sum(floats))
 expect_identical(as.numeric(sum(na)), NA_real_)
if (!grepl("devel", R.version.string)) {
# Valgrind on R-devel confuses NaN and NA_real_
 expect_identical(as.numeric(sum(na)), sum(floats))
 }
@jonkeanejonkeaneMay 4, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yeah, I'll add the devel check to skip_on_linux_cran() (and probably rename it too, though I'm a bit hesitant to make it so obvious that we are skipping the problematic tests — is that something we should worry about?).
As for these specific tests, looking at it now I wonder if we have benefit from expect_identical(as.numeric(sum(na)), sum(floats)) versus expect_identical(as.numeric(sum(na)), NA_real_)? Is there a circumstance where we expect sum(floats) to change behavior and we would want to match it? I find expect_identical(as.numeric(sum(na)), NA_real_) to be easier to read at a glance and see exactly what it's testing/expecting.
nealrichardson
nealrichardson
reviewed
May 4, 2021
View reviewed changes
Comment threaddocker-compose.yml
Outdated

@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 7f29abc988154513ad8f43a789b3399c3dfd971c
Submitted crossbow builds: ursacomputing/crossbow @ actions-384
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
(with mimalloc as default)
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: a94cff3c451e708e387d27dd89e9832b2eedbfea
Submitted crossbow builds: ursacomputing/crossbow @ actions-385
TaskStatus
test-r-linux-valgrindAzure
nealrichardson
nealrichardson
requested changes
May 4, 2021
View reviewed changes
@nealrichardsonnealrichardson
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Thanks a lot for taking this on.
Comment threadr/tests/testthat/helper-skip.R
Outdated

Comment threadr/tests/testthat/helper-skip.R
Outdated

Comment threaddocker-compose.yml
Outdated


Comment on lines

1072
 to 
1073

@nealrichardsonnealrichardsonMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

ARROW_USER_SIMD_LEVEL: "AVX2"
ARROW_RUNTIME_SIMD_LEVEL: "AVX2"
Comment threadci/scripts/r_valgrind.sh
Outdated

@nealrichardsonnealrichardsonMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Have you confirmed that this job (in the script's final form) correctly fails if there's a valgrind failure? You could try this by making skip_on_valgrind() no-op and see if it fails as expected, then restore the skips.
@jonkeanejonkeaneMay 4, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes it does, though I will re-confirm that once we are close to merging
@jonkeane
jonkeaneforce-pushed
the
ARROW-12571-valgrindCI-patch
branch
from
7886dcb to
5a7493eCompareMay 4, 2021 18:00
@jonkeane
jonkeane
changed the base branch from
release-4.0.0
to
masterMay 4, 2021 18:11
@jonkeane
jonkeaneforce-pushed
the
ARROW-12571-valgrindCI-patch
branch
from
5a7493e to
3ab8871CompareMay 4, 2021 18:11
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 3ab8871
Submitted crossbow builds: ursacomputing/crossbow @ actions-389
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
If ^^^ doesn't pass, there was a merge/squash error
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
We don't expect ^^^ to pass, it is intentionally broken
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 324786f
Submitted crossbow builds: ursacomputing/crossbow @ actions-390
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeaneforce-pushed
the
ARROW-12571-valgrindCI-patch
branch
from
e1a8bd3 to
3bb94e6CompareMay 4, 2021 21:18
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 3bb94e6
Submitted crossbow builds: ursacomputing/crossbow @ actions-391
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
We don't expect ^^^ to pass, it is intentionally broken
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
And ^^^ should pass now that we reverted our intentional failures.
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 5812a19
Submitted crossbow builds: ursacomputing/crossbow @ actions-392
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
deleted the

ARROW-12571-valgrindCI-patch

branch
May 5, 2021 12:51
jonkeane added a commit
to jonkeane/arrow
that referenced
this pull request
May 5, 2021
@jonkeane
ARROW-12571: [R][CI] Run nightly R with valgrind
1ced508
Closesapache#10237 from jonkeane/ARROW-12571-valgrindCI-patch
Authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
kszucs pushed a commit
to kszucs/arrow
that referenced
this pull request
May 17, 2021
@jonkeane@kszucs
ARROW-12571: [R][CI] Run nightly R with valgrind
be5f704
Closesapache#10237 from jonkeane/ARROW-12571-valgrindCI-patch
Authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
@asfimportasfimport
mentioned this pull request
May 5, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@pitroupitroupitrou left review comments
@nealrichardsonnealrichardsonnealrichardson approved these changes

Assignees


No one assigned


Labels

Component: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@jonkeane@pitrou@nealrichardson

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
ARROW-12571: [R][CI] Run nightly R with valgrind - #10237
Closed
jonkeane wants to merge 4 commits into
apache:masterfrom 
jonkeane:ARROW-12571-valgrindCI-patch
Closed
ARROW-12571: [R][CI] Run nightly R with valgrind#10237
jonkeane wants to merge 4 commits into
apache:masterfrom 
jonkeane:ARROW-12571-valgrindCI-patch
Conversation
@jonkeane
@jonkeanejonkeane
commented
May 3, 2021

Copy link


Copy Markdown

Member
 No description provided. 
@github-actions
github-actionsBot
commented
May 3, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-12571
@jonkeane
jonkeane
commented
May 3, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 3, 2021

Copy link


Copy Markdown

Revision: 0e1d0e219c3e05b52ad180bd626a499138b577d6
Submitted crossbow builds: ursacomputing/crossbow @ actions-377
TaskStatus
test-r-linux-valgrindAzure
pitrou
pitrou
reviewed
May 4, 2021
View reviewed changes
Comment threadci/scripts/r_valgrind.sh
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

You should use valgrind --error-exitcode=1 instead of grepping.
Comment threadci/scripts/r_valgrind.sh
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We should use a Valgrind suppressions file instead of grepping:
https://stackoverflow.com/questions/17159578/generating-suppressions-for-memory-leaks
Comment threadr/tests/testthat/test-compute-aggregate.R
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Why these changes? If they have a specific purpose, there should be a comment explaining them.
pitrou
pitrou
reviewed
May 4, 2021
View reviewed changes
Comment threadci/scripts/r_valgrind.sh
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Isn't this dangerous if there are CI secrets in environment variables? Why is this needed?
@jonkeanejonkeaneMay 4, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I was using it to diagnose if the ARROW_RUNTIME_SIMD_LEVEL var was being passed — secrets should be redacted even with something like printenv, but I've removed it regardless.
@pitrou
pitrou
commented
May 4, 2021

Copy link


Copy Markdown

Member
I'm trying this out and I see that it takes a lot of time without outputting anything:
+ RDvalgrind --vanilla -d 'valgrind --tool=memcheck --leak-check=full --track-origins=yes' -f testthat.R

Can the above command output progress while running the test suite?
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@jonkeanejonkeane
closed this
May 4, 2021
@jonkeanejonkeane
reopened this
May 4, 2021
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 041f4095fb71adfbfb5d504ff83da04c07cb4352
Submitted crossbow builds: ursacomputing/crossbow @ actions-379
TaskStatus
test-r-linux-valgrindAzure
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 041f4095fb71adfbfb5d504ff83da04c07cb4352
Submitted crossbow builds: ursacomputing/crossbow @ actions-380
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 4ede120d38854d6bbf7556450ed15e8345b59fcd
Submitted crossbow builds: ursacomputing/crossbow @ actions-381
TaskStatus
test-r-linux-valgrindAzure
nealrichardson
nealrichardson
reviewed
May 4, 2021
View reviewed changes
Comment threadr/tests/testthat/test-compute-aggregate.R
Outdated


Comment on lines

40
 to 
41

@nealrichardsonnealrichardsonMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Could also do something like (of course confirming the right way to determine a devel version). Could also add the devel check to the skip_on_linux_cran (and call it skip_on_valgrind()) if you wanted to make that narrower.

Suggested change

# expect_identical(as.numeric(sum(na)), sum(floats))
 expect_identical(as.numeric(sum(na)), NA_real_)
if (!grepl("devel", R.version.string)) {
# Valgrind on R-devel confuses NaN and NA_real_
 expect_identical(as.numeric(sum(na)), sum(floats))
 }
@jonkeanejonkeaneMay 4, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yeah, I'll add the devel check to skip_on_linux_cran() (and probably rename it too, though I'm a bit hesitant to make it so obvious that we are skipping the problematic tests — is that something we should worry about?).
As for these specific tests, looking at it now I wonder if we have benefit from expect_identical(as.numeric(sum(na)), sum(floats)) versus expect_identical(as.numeric(sum(na)), NA_real_)? Is there a circumstance where we expect sum(floats) to change behavior and we would want to match it? I find expect_identical(as.numeric(sum(na)), NA_real_) to be easier to read at a glance and see exactly what it's testing/expecting.
nealrichardson
nealrichardson
reviewed
May 4, 2021
View reviewed changes
Comment threaddocker-compose.yml
Outdated

@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 7f29abc988154513ad8f43a789b3399c3dfd971c
Submitted crossbow builds: ursacomputing/crossbow @ actions-384
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
(with mimalloc as default)
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: a94cff3c451e708e387d27dd89e9832b2eedbfea
Submitted crossbow builds: ursacomputing/crossbow @ actions-385
TaskStatus
test-r-linux-valgrindAzure
nealrichardson
nealrichardson
requested changes
May 4, 2021
View reviewed changes
@nealrichardsonnealrichardson
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Thanks a lot for taking this on.
Comment threadr/tests/testthat/helper-skip.R
Outdated

Comment threadr/tests/testthat/helper-skip.R
Outdated

Comment threaddocker-compose.yml
Outdated


Comment on lines

1072
 to 
1073

@nealrichardsonnealrichardsonMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

ARROW_USER_SIMD_LEVEL: "AVX2"
ARROW_RUNTIME_SIMD_LEVEL: "AVX2"
Comment threadci/scripts/r_valgrind.sh
Outdated

@nealrichardsonnealrichardsonMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Have you confirmed that this job (in the script's final form) correctly fails if there's a valgrind failure? You could try this by making skip_on_valgrind() no-op and see if it fails as expected, then restore the skips.
@jonkeanejonkeaneMay 4, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes it does, though I will re-confirm that once we are close to merging
@jonkeane
jonkeaneforce-pushed
the
ARROW-12571-valgrindCI-patch
branch
from
7886dcb to
5a7493eCompareMay 4, 2021 18:00
@jonkeane
jonkeane
changed the base branch from
release-4.0.0
to
masterMay 4, 2021 18:11
@jonkeane
jonkeaneforce-pushed
the
ARROW-12571-valgrindCI-patch
branch
from
5a7493e to
3ab8871CompareMay 4, 2021 18:11
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 3ab8871
Submitted crossbow builds: ursacomputing/crossbow @ actions-389
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
If ^^^ doesn't pass, there was a merge/squash error
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
We don't expect ^^^ to pass, it is intentionally broken
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 324786f
Submitted crossbow builds: ursacomputing/crossbow @ actions-390
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeaneforce-pushed
the
ARROW-12571-valgrindCI-patch
branch
from
e1a8bd3 to
3bb94e6CompareMay 4, 2021 21:18
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 3bb94e6
Submitted crossbow builds: ursacomputing/crossbow @ actions-391
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
We don't expect ^^^ to pass, it is intentionally broken
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
And ^^^ should pass now that we reverted our intentional failures.
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 5812a19
Submitted crossbow builds: ursacomputing/crossbow @ actions-392
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
deleted the

ARROW-12571-valgrindCI-patch

branch
May 5, 2021 12:51
jonkeane added a commit
to jonkeane/arrow
that referenced
this pull request
May 5, 2021
@jonkeane
ARROW-12571: [R][CI] Run nightly R with valgrind
1ced508
Closesapache#10237 from jonkeane/ARROW-12571-valgrindCI-patch
Authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
kszucs pushed a commit
to kszucs/arrow
that referenced
this pull request
May 17, 2021
@jonkeane@kszucs
ARROW-12571: [R][CI] Run nightly R with valgrind
be5f704
Closesapache#10237 from jonkeane/ARROW-12571-valgrindCI-patch
Authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
@asfimportasfimport
mentioned this pull request
May 5, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@pitroupitroupitrou left review comments
@nealrichardsonnealrichardsonnealrichardson approved these changes

Assignees


No one assigned


Labels

Component: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@jonkeane@pitrou@nealrichardson

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-12571: [R][CI] Run nightly R with valgrind - #10237
Closed
jonkeane wants to merge 4 commits into
apache:masterfrom 
jonkeane:ARROW-12571-valgrindCI-patch
Closed
ARROW-12571: [R][CI] Run nightly R with valgrind#10237
jonkeane wants to merge 4 commits into
apache:masterfrom 
jonkeane:ARROW-12571-valgrindCI-patch
Conversation
@jonkeane
@jonkeanejonkeane
commented
May 3, 2021

Copy link


Copy Markdown

Member
 No description provided. 
@github-actions
github-actionsBot
commented
May 3, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-12571
@jonkeane
jonkeane
commented
May 3, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 3, 2021

Copy link


Copy Markdown

Revision: 0e1d0e219c3e05b52ad180bd626a499138b577d6
Submitted crossbow builds: ursacomputing/crossbow @ actions-377
TaskStatus
test-r-linux-valgrindAzure
pitrou
pitrou
reviewed
May 4, 2021
View reviewed changes
Comment threadci/scripts/r_valgrind.sh
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

You should use valgrind --error-exitcode=1 instead of grepping.
Comment threadci/scripts/r_valgrind.sh
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We should use a Valgrind suppressions file instead of grepping:
https://stackoverflow.com/questions/17159578/generating-suppressions-for-memory-leaks
Comment threadr/tests/testthat/test-compute-aggregate.R
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Why these changes? If they have a specific purpose, there should be a comment explaining them.
pitrou
pitrou
reviewed
May 4, 2021
View reviewed changes
Comment threadci/scripts/r_valgrind.sh
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Isn't this dangerous if there are CI secrets in environment variables? Why is this needed?
@jonkeanejonkeaneMay 4, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I was using it to diagnose if the ARROW_RUNTIME_SIMD_LEVEL var was being passed — secrets should be redacted even with something like printenv, but I've removed it regardless.
@pitrou
pitrou
commented
May 4, 2021

Copy link


Copy Markdown

Member
I'm trying this out and I see that it takes a lot of time without outputting anything:
+ RDvalgrind --vanilla -d 'valgrind --tool=memcheck --leak-check=full --track-origins=yes' -f testthat.R

Can the above command output progress while running the test suite?
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@jonkeanejonkeane
closed this
May 4, 2021
@jonkeanejonkeane
reopened this
May 4, 2021
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 041f4095fb71adfbfb5d504ff83da04c07cb4352
Submitted crossbow builds: ursacomputing/crossbow @ actions-379
TaskStatus
test-r-linux-valgrindAzure
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 041f4095fb71adfbfb5d504ff83da04c07cb4352
Submitted crossbow builds: ursacomputing/crossbow @ actions-380
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 4ede120d38854d6bbf7556450ed15e8345b59fcd
Submitted crossbow builds: ursacomputing/crossbow @ actions-381
TaskStatus
test-r-linux-valgrindAzure
nealrichardson
nealrichardson
reviewed
May 4, 2021
View reviewed changes
Comment threadr/tests/testthat/test-compute-aggregate.R
Outdated


Comment on lines

40
 to 
41

@nealrichardsonnealrichardsonMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Could also do something like (of course confirming the right way to determine a devel version). Could also add the devel check to the skip_on_linux_cran (and call it skip_on_valgrind()) if you wanted to make that narrower.

Suggested change

# expect_identical(as.numeric(sum(na)), sum(floats))
 expect_identical(as.numeric(sum(na)), NA_real_)
if (!grepl("devel", R.version.string)) {
# Valgrind on R-devel confuses NaN and NA_real_
 expect_identical(as.numeric(sum(na)), sum(floats))
 }
@jonkeanejonkeaneMay 4, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yeah, I'll add the devel check to skip_on_linux_cran() (and probably rename it too, though I'm a bit hesitant to make it so obvious that we are skipping the problematic tests — is that something we should worry about?).
As for these specific tests, looking at it now I wonder if we have benefit from expect_identical(as.numeric(sum(na)), sum(floats)) versus expect_identical(as.numeric(sum(na)), NA_real_)? Is there a circumstance where we expect sum(floats) to change behavior and we would want to match it? I find expect_identical(as.numeric(sum(na)), NA_real_) to be easier to read at a glance and see exactly what it's testing/expecting.
nealrichardson
nealrichardson
reviewed
May 4, 2021
View reviewed changes
Comment threaddocker-compose.yml
Outdated

@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 7f29abc988154513ad8f43a789b3399c3dfd971c
Submitted crossbow builds: ursacomputing/crossbow @ actions-384
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
(with mimalloc as default)
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: a94cff3c451e708e387d27dd89e9832b2eedbfea
Submitted crossbow builds: ursacomputing/crossbow @ actions-385
TaskStatus
test-r-linux-valgrindAzure
nealrichardson
nealrichardson
requested changes
May 4, 2021
View reviewed changes
@nealrichardsonnealrichardson
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Thanks a lot for taking this on.
Comment threadr/tests/testthat/helper-skip.R
Outdated

Comment threadr/tests/testthat/helper-skip.R
Outdated

Comment threaddocker-compose.yml
Outdated


Comment on lines

1072
 to 
1073

@nealrichardsonnealrichardsonMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

ARROW_USER_SIMD_LEVEL: "AVX2"
ARROW_RUNTIME_SIMD_LEVEL: "AVX2"
Comment threadci/scripts/r_valgrind.sh
Outdated

@nealrichardsonnealrichardsonMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Have you confirmed that this job (in the script's final form) correctly fails if there's a valgrind failure? You could try this by making skip_on_valgrind() no-op and see if it fails as expected, then restore the skips.
@jonkeanejonkeaneMay 4, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes it does, though I will re-confirm that once we are close to merging
@jonkeane
jonkeaneforce-pushed
the
ARROW-12571-valgrindCI-patch
branch
from
7886dcb to
5a7493eCompareMay 4, 2021 18:00
@jonkeane
jonkeane
changed the base branch from
release-4.0.0
to
masterMay 4, 2021 18:11
@jonkeane
jonkeaneforce-pushed
the
ARROW-12571-valgrindCI-patch
branch
from
5a7493e to
3ab8871CompareMay 4, 2021 18:11
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 3ab8871
Submitted crossbow builds: ursacomputing/crossbow @ actions-389
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
If ^^^ doesn't pass, there was a merge/squash error
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
We don't expect ^^^ to pass, it is intentionally broken
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 324786f
Submitted crossbow builds: ursacomputing/crossbow @ actions-390
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeaneforce-pushed
the
ARROW-12571-valgrindCI-patch
branch
from
e1a8bd3 to
3bb94e6CompareMay 4, 2021 21:18
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 3bb94e6
Submitted crossbow builds: ursacomputing/crossbow @ actions-391
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
We don't expect ^^^ to pass, it is intentionally broken
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
And ^^^ should pass now that we reverted our intentional failures.
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 5812a19
Submitted crossbow builds: ursacomputing/crossbow @ actions-392
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
deleted the

ARROW-12571-valgrindCI-patch

branch
May 5, 2021 12:51
jonkeane added a commit
to jonkeane/arrow
that referenced
this pull request
May 5, 2021
@jonkeane
ARROW-12571: [R][CI] Run nightly R with valgrind
1ced508
Closesapache#10237 from jonkeane/ARROW-12571-valgrindCI-patch
Authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
kszucs pushed a commit
to kszucs/arrow
that referenced
this pull request
May 17, 2021
@jonkeane@kszucs
ARROW-12571: [R][CI] Run nightly R with valgrind
be5f704
Closesapache#10237 from jonkeane/ARROW-12571-valgrindCI-patch
Authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
@asfimportasfimport
mentioned this pull request
May 5, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@pitroupitroupitrou left review comments
@nealrichardsonnealrichardsonnealrichardson approved these changes

Assignees


No one assigned


Labels

Component: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@jonkeane@pitrou@nealrichardson

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-12571: [R][CI] Run nightly R with valgrind - #10237
Closed
jonkeane wants to merge 4 commits into
apache:masterfrom 
jonkeane:ARROW-12571-valgrindCI-patch
Closed
ARROW-12571: [R][CI] Run nightly R with valgrind#10237
jonkeane wants to merge 4 commits into
apache:masterfrom 
jonkeane:ARROW-12571-valgrindCI-patch
Conversation
@jonkeane
@jonkeanejonkeane
commented
May 3, 2021

Copy link


Copy Markdown

Member
 No description provided. 
@github-actions
github-actionsBot
commented
May 3, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-12571
@jonkeane
jonkeane
commented
May 3, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 3, 2021

Copy link


Copy Markdown

Revision: 0e1d0e219c3e05b52ad180bd626a499138b577d6
Submitted crossbow builds: ursacomputing/crossbow @ actions-377
TaskStatus
test-r-linux-valgrindAzure
pitrou
pitrou
reviewed
May 4, 2021
View reviewed changes
Comment threadci/scripts/r_valgrind.sh
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

You should use valgrind --error-exitcode=1 instead of grepping.
Comment threadci/scripts/r_valgrind.sh
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We should use a Valgrind suppressions file instead of grepping:
https://stackoverflow.com/questions/17159578/generating-suppressions-for-memory-leaks
Comment threadr/tests/testthat/test-compute-aggregate.R
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Why these changes? If they have a specific purpose, there should be a comment explaining them.
pitrou
pitrou
reviewed
May 4, 2021
View reviewed changes
Comment threadci/scripts/r_valgrind.sh
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Isn't this dangerous if there are CI secrets in environment variables? Why is this needed?
@jonkeanejonkeaneMay 4, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I was using it to diagnose if the ARROW_RUNTIME_SIMD_LEVEL var was being passed — secrets should be redacted even with something like printenv, but I've removed it regardless.
@pitrou
pitrou
commented
May 4, 2021

Copy link


Copy Markdown

Member
I'm trying this out and I see that it takes a lot of time without outputting anything:
+ RDvalgrind --vanilla -d 'valgrind --tool=memcheck --leak-check=full --track-origins=yes' -f testthat.R

Can the above command output progress while running the test suite?
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@jonkeanejonkeane
closed this
May 4, 2021
@jonkeanejonkeane
reopened this
May 4, 2021
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 041f4095fb71adfbfb5d504ff83da04c07cb4352
Submitted crossbow builds: ursacomputing/crossbow @ actions-379
TaskStatus
test-r-linux-valgrindAzure
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 041f4095fb71adfbfb5d504ff83da04c07cb4352
Submitted crossbow builds: ursacomputing/crossbow @ actions-380
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 4ede120d38854d6bbf7556450ed15e8345b59fcd
Submitted crossbow builds: ursacomputing/crossbow @ actions-381
TaskStatus
test-r-linux-valgrindAzure
nealrichardson
nealrichardson
reviewed
May 4, 2021
View reviewed changes
Comment threadr/tests/testthat/test-compute-aggregate.R
Outdated


Comment on lines

40
 to 
41

@nealrichardsonnealrichardsonMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Could also do something like (of course confirming the right way to determine a devel version). Could also add the devel check to the skip_on_linux_cran (and call it skip_on_valgrind()) if you wanted to make that narrower.

Suggested change

# expect_identical(as.numeric(sum(na)), sum(floats))
 expect_identical(as.numeric(sum(na)), NA_real_)
if (!grepl("devel", R.version.string)) {
# Valgrind on R-devel confuses NaN and NA_real_
 expect_identical(as.numeric(sum(na)), sum(floats))
 }
@jonkeanejonkeaneMay 4, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yeah, I'll add the devel check to skip_on_linux_cran() (and probably rename it too, though I'm a bit hesitant to make it so obvious that we are skipping the problematic tests — is that something we should worry about?).
As for these specific tests, looking at it now I wonder if we have benefit from expect_identical(as.numeric(sum(na)), sum(floats)) versus expect_identical(as.numeric(sum(na)), NA_real_)? Is there a circumstance where we expect sum(floats) to change behavior and we would want to match it? I find expect_identical(as.numeric(sum(na)), NA_real_) to be easier to read at a glance and see exactly what it's testing/expecting.
nealrichardson
nealrichardson
reviewed
May 4, 2021
View reviewed changes
Comment threaddocker-compose.yml
Outdated

@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 7f29abc988154513ad8f43a789b3399c3dfd971c
Submitted crossbow builds: ursacomputing/crossbow @ actions-384
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
(with mimalloc as default)
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: a94cff3c451e708e387d27dd89e9832b2eedbfea
Submitted crossbow builds: ursacomputing/crossbow @ actions-385
TaskStatus
test-r-linux-valgrindAzure
nealrichardson
nealrichardson
requested changes
May 4, 2021
View reviewed changes
@nealrichardsonnealrichardson
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Thanks a lot for taking this on.
Comment threadr/tests/testthat/helper-skip.R
Outdated

Comment threadr/tests/testthat/helper-skip.R
Outdated

Comment threaddocker-compose.yml
Outdated


Comment on lines

1072
 to 
1073

@nealrichardsonnealrichardsonMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

ARROW_USER_SIMD_LEVEL: "AVX2"
ARROW_RUNTIME_SIMD_LEVEL: "AVX2"
Comment threadci/scripts/r_valgrind.sh
Outdated

@nealrichardsonnealrichardsonMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Have you confirmed that this job (in the script's final form) correctly fails if there's a valgrind failure? You could try this by making skip_on_valgrind() no-op and see if it fails as expected, then restore the skips.
@jonkeanejonkeaneMay 4, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes it does, though I will re-confirm that once we are close to merging
@jonkeane
jonkeaneforce-pushed
the
ARROW-12571-valgrindCI-patch
branch
from
7886dcb to
5a7493eCompareMay 4, 2021 18:00
@jonkeane
jonkeane
changed the base branch from
release-4.0.0
to
masterMay 4, 2021 18:11
@jonkeane
jonkeaneforce-pushed
the
ARROW-12571-valgrindCI-patch
branch
from
5a7493e to
3ab8871CompareMay 4, 2021 18:11
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 3ab8871
Submitted crossbow builds: ursacomputing/crossbow @ actions-389
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
If ^^^ doesn't pass, there was a merge/squash error
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
We don't expect ^^^ to pass, it is intentionally broken
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 324786f
Submitted crossbow builds: ursacomputing/crossbow @ actions-390
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeaneforce-pushed
the
ARROW-12571-valgrindCI-patch
branch
from
e1a8bd3 to
3bb94e6CompareMay 4, 2021 21:18
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 3bb94e6
Submitted crossbow builds: ursacomputing/crossbow @ actions-391
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
We don't expect ^^^ to pass, it is intentionally broken
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
And ^^^ should pass now that we reverted our intentional failures.
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 5812a19
Submitted crossbow builds: ursacomputing/crossbow @ actions-392
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
deleted the

ARROW-12571-valgrindCI-patch

branch
May 5, 2021 12:51
jonkeane added a commit
to jonkeane/arrow
that referenced
this pull request
May 5, 2021
@jonkeane
ARROW-12571: [R][CI] Run nightly R with valgrind
1ced508
Closesapache#10237 from jonkeane/ARROW-12571-valgrindCI-patch
Authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
kszucs pushed a commit
to kszucs/arrow
that referenced
this pull request
May 17, 2021
@jonkeane@kszucs
ARROW-12571: [R][CI] Run nightly R with valgrind
be5f704
Closesapache#10237 from jonkeane/ARROW-12571-valgrindCI-patch
Authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
@asfimportasfimport
mentioned this pull request
May 5, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@pitroupitroupitrou left review comments
@nealrichardsonnealrichardsonnealrichardson approved these changes

Assignees


No one assigned


Labels

Component: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@jonkeane@pitrou@nealrichardson

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
ARROW-12571: [R][CI] Run nightly R with valgrind - #10237
Closed
jonkeane wants to merge 4 commits into
apache:masterfrom 
jonkeane:ARROW-12571-valgrindCI-patch
Closed
ARROW-12571: [R][CI] Run nightly R with valgrind#10237
jonkeane wants to merge 4 commits into
apache:masterfrom 
jonkeane:ARROW-12571-valgrindCI-patch
Conversation
@jonkeane
@jonkeanejonkeane
commented
May 3, 2021

Copy link


Copy Markdown

Member
 No description provided. 
@github-actions
github-actionsBot
commented
May 3, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-12571
@jonkeane
jonkeane
commented
May 3, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 3, 2021

Copy link


Copy Markdown

Revision: 0e1d0e219c3e05b52ad180bd626a499138b577d6
Submitted crossbow builds: ursacomputing/crossbow @ actions-377
TaskStatus
test-r-linux-valgrindAzure
pitrou
pitrou
reviewed
May 4, 2021
View reviewed changes
Comment threadci/scripts/r_valgrind.sh
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

You should use valgrind --error-exitcode=1 instead of grepping.
Comment threadci/scripts/r_valgrind.sh
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We should use a Valgrind suppressions file instead of grepping:
https://stackoverflow.com/questions/17159578/generating-suppressions-for-memory-leaks
Comment threadr/tests/testthat/test-compute-aggregate.R
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Why these changes? If they have a specific purpose, there should be a comment explaining them.
pitrou
pitrou
reviewed
May 4, 2021
View reviewed changes
Comment threadci/scripts/r_valgrind.sh
Outdated

@pitroupitrouMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Isn't this dangerous if there are CI secrets in environment variables? Why is this needed?
@jonkeanejonkeaneMay 4, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I was using it to diagnose if the ARROW_RUNTIME_SIMD_LEVEL var was being passed — secrets should be redacted even with something like printenv, but I've removed it regardless.
@pitrou
pitrou
commented
May 4, 2021

Copy link


Copy Markdown

Member
I'm trying this out and I see that it takes a lot of time without outputting anything:
+ RDvalgrind --vanilla -d 'valgrind --tool=memcheck --leak-check=full --track-origins=yes' -f testthat.R

Can the above command output progress while running the test suite?
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@jonkeanejonkeane
closed this
May 4, 2021
@jonkeanejonkeane
reopened this
May 4, 2021
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 041f4095fb71adfbfb5d504ff83da04c07cb4352
Submitted crossbow builds: ursacomputing/crossbow @ actions-379
TaskStatus
test-r-linux-valgrindAzure
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 041f4095fb71adfbfb5d504ff83da04c07cb4352
Submitted crossbow builds: ursacomputing/crossbow @ actions-380
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 4ede120d38854d6bbf7556450ed15e8345b59fcd
Submitted crossbow builds: ursacomputing/crossbow @ actions-381
TaskStatus
test-r-linux-valgrindAzure
nealrichardson
nealrichardson
reviewed
May 4, 2021
View reviewed changes
Comment threadr/tests/testthat/test-compute-aggregate.R
Outdated


Comment on lines

40
 to 
41

@nealrichardsonnealrichardsonMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Could also do something like (of course confirming the right way to determine a devel version). Could also add the devel check to the skip_on_linux_cran (and call it skip_on_valgrind()) if you wanted to make that narrower.

Suggested change

# expect_identical(as.numeric(sum(na)), sum(floats))
 expect_identical(as.numeric(sum(na)), NA_real_)
if (!grepl("devel", R.version.string)) {
# Valgrind on R-devel confuses NaN and NA_real_
 expect_identical(as.numeric(sum(na)), sum(floats))
 }
@jonkeanejonkeaneMay 4, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yeah, I'll add the devel check to skip_on_linux_cran() (and probably rename it too, though I'm a bit hesitant to make it so obvious that we are skipping the problematic tests — is that something we should worry about?).
As for these specific tests, looking at it now I wonder if we have benefit from expect_identical(as.numeric(sum(na)), sum(floats)) versus expect_identical(as.numeric(sum(na)), NA_real_)? Is there a circumstance where we expect sum(floats) to change behavior and we would want to match it? I find expect_identical(as.numeric(sum(na)), NA_real_) to be easier to read at a glance and see exactly what it's testing/expecting.
nealrichardson
nealrichardson
reviewed
May 4, 2021
View reviewed changes
Comment threaddocker-compose.yml
Outdated

@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 7f29abc988154513ad8f43a789b3399c3dfd971c
Submitted crossbow builds: ursacomputing/crossbow @ actions-384
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
(with mimalloc as default)
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: a94cff3c451e708e387d27dd89e9832b2eedbfea
Submitted crossbow builds: ursacomputing/crossbow @ actions-385
TaskStatus
test-r-linux-valgrindAzure
nealrichardson
nealrichardson
requested changes
May 4, 2021
View reviewed changes
@nealrichardsonnealrichardson
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Thanks a lot for taking this on.
Comment threadr/tests/testthat/helper-skip.R
Outdated

Comment threadr/tests/testthat/helper-skip.R
Outdated

Comment threaddocker-compose.yml
Outdated


Comment on lines

1072
 to 
1073

@nealrichardsonnealrichardsonMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

ARROW_USER_SIMD_LEVEL: "AVX2"
ARROW_RUNTIME_SIMD_LEVEL: "AVX2"
Comment threadci/scripts/r_valgrind.sh
Outdated

@nealrichardsonnealrichardsonMay 4, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Have you confirmed that this job (in the script's final form) correctly fails if there's a valgrind failure? You could try this by making skip_on_valgrind() no-op and see if it fails as expected, then restore the skips.
@jonkeanejonkeaneMay 4, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes it does, though I will re-confirm that once we are close to merging
@jonkeane
jonkeaneforce-pushed
the
ARROW-12571-valgrindCI-patch
branch
from
7886dcb to
5a7493eCompareMay 4, 2021 18:00
@jonkeane
jonkeane
changed the base branch from
release-4.0.0
to
masterMay 4, 2021 18:11
@jonkeane
jonkeaneforce-pushed
the
ARROW-12571-valgrindCI-patch
branch
from
5a7493e to
3ab8871CompareMay 4, 2021 18:11
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 3ab8871
Submitted crossbow builds: ursacomputing/crossbow @ actions-389
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
If ^^^ doesn't pass, there was a merge/squash error
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
We don't expect ^^^ to pass, it is intentionally broken
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 324786f
Submitted crossbow builds: ursacomputing/crossbow @ actions-390
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeaneforce-pushed
the
ARROW-12571-valgrindCI-patch
branch
from
e1a8bd3 to
3bb94e6CompareMay 4, 2021 21:18
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 3bb94e6
Submitted crossbow builds: ursacomputing/crossbow @ actions-391
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
We don't expect ^^^ to pass, it is intentionally broken
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-r-linux-valgrind
@jonkeane
jonkeane
commented
May 4, 2021

Copy link


Copy Markdown

MemberAuthor
And ^^^ should pass now that we reverted our intentional failures.
@github-actions
github-actionsBot
commented
May 4, 2021

Copy link


Copy Markdown

Revision: 5812a19
Submitted crossbow builds: ursacomputing/crossbow @ actions-392
TaskStatus
test-r-linux-valgrindAzure
@jonkeane
jonkeane
deleted the

ARROW-12571-valgrindCI-patch

branch
May 5, 2021 12:51
jonkeane added a commit
to jonkeane/arrow
that referenced
this pull request
May 5, 2021
@jonkeane
ARROW-12571: [R][CI] Run nightly R with valgrind
1ced508
Closesapache#10237 from jonkeane/ARROW-12571-valgrindCI-patch
Authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
kszucs pushed a commit
to kszucs/arrow
that referenced
this pull request
May 17, 2021
@jonkeane@kszucs
ARROW-12571: [R][CI] Run nightly R with valgrind
be5f704
Closesapache#10237 from jonkeane/ARROW-12571-valgrindCI-patch
Authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
@asfimportasfimport
mentioned this pull request
May 5, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@pitroupitroupitrou left review comments
@nealrichardsonnealrichardsonnealrichardson approved these changes

Assignees


No one assigned


Labels

Component: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@jonkeane@pitrou@nealrichardson