Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add in neverReplaceExec and several rules for it #660

Merged
merged 1 commit into from
Sep 4, 2020

Conversation

revans2
Copy link
Collaborator

@revans2 revans2 commented Sep 3, 2020

This extends the great work done in #647 but fixes an issue there where we were still outputting config documentation for execs we never plan to replace.

I also made adding one of these in simpler and more common. I then added in rules for all of the other commands we currently don't want to warn about or ever replace.

This fixes #499

@revans2 revans2 added the feature request New feature or request label Sep 3, 2020
@revans2 revans2 added this to the Aug 31 - Sep 11 milestone Sep 3, 2020
@revans2 revans2 requested a review from andygrove September 3, 2020 21:59
@revans2 revans2 self-assigned this Sep 3, 2020
@revans2
Copy link
Collaborator Author

revans2 commented Sep 3, 2020

build

Copy link
Contributor

@andygrove andygrove left a comment

Choose a reason for hiding this comment

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

Thanks, that's much nicer. LGTM.

@jlowe
Copy link
Member

jlowe commented Sep 3, 2020

fixes an issue there where we were still outputting config documentation for execs we never plan to replace.

Curious, I expected to see some deletes in configs.md as part of this. Did we miss the configs.md changes from the previous PR somehow?

@revans2
Copy link
Collaborator Author

revans2 commented Sep 3, 2020

Curious, I expected to see some deletes in configs.md as part of this. Did we miss the configs.md changes from the previous PR somehow?

Yup if there had been there I would not have approved it, but they showed up afterward for me as a part of another build, so I thought why not fix it and also knock out #499 too while I was at it.

@jlowe
Copy link
Member

jlowe commented Sep 3, 2020

We probably need a CI check for docs being modified during the build (maybe any checked out file being modified?), indicating PR should have included some changes that are missing. I'll file an issue.

@revans2 revans2 merged commit a0101d1 into NVIDIA:branch-0.2 Sep 4, 2020
nartal1 pushed a commit to nartal1/spark-rapids that referenced this pull request Jun 9, 2021
nartal1 pushed a commit to nartal1/spark-rapids that referenced this pull request Jun 9, 2021
tgravescs pushed a commit to tgravescs/spark-rapids that referenced this pull request Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] disable any kind of warnings about ExecutedCommandExec not being on the GPU
3 participants