Allow control of --check-bounds command option #46

IanButterworth · 2021-07-06T02:19:08Z

I believe this command option is a bit more complicated than setting --check-bounds to yes (force always) or no (force never), because the default state is a 3rd state where --check-bounds is omitted entirely, and bounds checking is done per code decisions.. It'd be good if someone could confirm my understanding on that though.. the code is a bit hard to follow.

This is untested. I've not figured out how to test this in an action yet

IanButterworth · 2021-07-06T02:34:24Z

I'm not sure how to fix the bash here. Hopefully it's clear what i'm trying to do

codecov-commenter · 2021-07-06T04:07:13Z

Codecov Report

Merging #46 (8603d27) into master (f8a636d) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master       #46   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines            2         2           
=========================================
  Hits             2         2

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5f2948...8603d27. Read the comment docs.

IanButterworth · 2021-07-06T04:13:58Z

Ok. Should be fixed now

DilumAluthge · 2021-07-06T15:23:30Z

Can you squash the commits?

At a glance, it looks right to me, but I'm not the most familiar with this repo, so it would be good to get an approval from someone that knows the code better than I do.

SaschaMann · 2021-07-06T16:25:22Z

I'll take a look later. I can squash it on merge, no need for it now.

SaschaMann

The script looks alright, but I'm not sure about the behaviour of bounds checks.

Could you perhaps run the action thrice in a row for a package/script with all three inputs to see if it fails/behaves as expected as a demo?

Perhaps in a fork of https://github.com/julia-actions/Example.jl/blob/action-tests/test/runtests.jl?

action.yml

IanButterworth · 2021-07-08T14:36:08Z

Bounds check behavior confirmed on slack to be

--check-bounds=yes: force always bounds check
--check-bounds=no: force never bounds check
no command opt: automatic based on code controls

Happy to test run this with each config. How can I use this PR of the action though?

Co-authored-by: Sascha Mann <git@mail.saschamann.eu>

SaschaMann

Happy to test run this with each config. How can I use this PR of the action though?

Replace - uses: julia-actions/julia-runtest@v1 with - uses: IanButterworth/julia-runtest@ib/bounds_check_control in the workflow in the repo/package you want to run the tests in. Doesn't have to be merged, you can replace and run it on a branch.

action.yml

Co-authored-by: Sascha Mann <git@mail.saschamann.eu>

IanButterworth · 2021-07-12T03:52:43Z

Needs a fix in Pkg, as it turns out --check-bounds=yes is hardcoded into the Pkg test sandbox JuliaLang/Pkg.jl#2668 and there's no way to override it back to default currently.

Would be easier with JuliaLang/julia#41551

SaschaMann · 2021-07-13T06:39:54Z

Let's wait for that to be resolved then

IanButterworth · 2021-07-13T17:28:31Z

This PR IanButterworth/SystemBenchmark.jl#50 is running on [1, nightly] with each of the check bounds options.

On nightly it is respected, and can be seen in the command args shown at the start of the test suite.

However on 1 (1.6) given [pr link deleted] wasn't backported to 1.6, --check-bounds=yes cannot be overridden, but the tooling here doesn't error.
Backporting that PR for this alone is probably not the best course of action, so perhaps this is just for 1.7 and up

Turned out to be a bug in this PR.

Update: This PR is now failing where I would expect it to fail (until JuliaLang/julia#41551 is backported to 1.6)

kwargs.jl

Co-authored-by: Sascha Mann <git@mail.saschamann.eu>

IanButterworth · 2021-07-17T16:48:50Z

@SaschaMann Should be ready to merge as this works on nightly, and the backports of JuliaLang/julia#41551 are labelled

SaschaMann · 2021-07-18T08:26:35Z

kwargs.jl

+    elseif julia_args != ``
+        println("::warning::The `julia_args` option requires at least Julia 1.6. VERSION=$VERSION, julia_args=$julia_args")
+    end


Sorry to ask again, I want to be sure to understand this before merging: Can julia_args ever be `` here? It's set to julia_args = ["--check-bounds=${{ inputs.check_bounds }}"] in action.yml and ${{ inputs.check_bounds }} defaults to yes.

Good catch. Should be fixed now. The change makes kwargs.jl more specific to how it's used in this action, which I assumed wasn't a problem

SaschaMann · 2021-08-11T22:14:56Z

Sorry, I haven't had time to look at this again yet, it's on my list for the weekend.

c42f

I needed this today, so I had a read through the code.

Looks good to me!

kwargs.jl

Co-authored-by: Chris Foster <chris42f@gmail.com>

IanButterworth · 2021-12-14T00:32:16Z

It'd be nice to get this in

kwargs.jl

SaschaMann · 2021-12-22T14:00:13Z

Sorry for the delay

IanButterworth requested a review from a team as a code owner July 6, 2021 02:19

IanButterworth force-pushed the ib/bounds_check_control branch from c5537d8 to 607d74c Compare July 6, 2021 02:20

DilumAluthge requested review from SaschaMann and removed request for a team July 6, 2021 02:20

IanButterworth force-pushed the ib/bounds_check_control branch 2 times, most recently from 58fc1e0 to 17f2a09 Compare July 6, 2021 02:26

allow control of bounds-check command option

53c01e3

IanButterworth force-pushed the ib/bounds_check_control branch from 17f2a09 to 53c01e3 Compare July 6, 2021 02:29

tryfix

17b98dd

DilumAluthge requested review from christopher-dG and davidanthoff July 6, 2021 15:22

SaschaMann reviewed Jul 8, 2021

View reviewed changes

action.yml Outdated Show resolved Hide resolved

Update action.yml

b650dee

Co-authored-by: Sascha Mann <git@mail.saschamann.eu>

SaschaMann reviewed Jul 8, 2021

View reviewed changes

action.yml Outdated Show resolved Hide resolved

IanButterworth and others added 4 commits July 8, 2021 12:30

Update action.yml

8ec6545

Co-authored-by: Sascha Mann <git@mail.saschamann.eu>

tryfix

377a818

alternative bash if else approach

aa1ae13

another approach

9c0cb0d

IanButterworth mentioned this pull request Jul 12, 2021

add default "auto" as option to --check-bounds JuliaLang/julia#41551

Merged

IanButterworth added 3 commits July 13, 2021 12:44

use julia_args approach

ca36417

add julia_args to kwargs func

c12547f

fix

41251bb

IanButterworth added 3 commits July 13, 2021 13:41

move julia_args to before return

513b54d

guard against v1.0

26bfe8e

add warning

f2e7be0

SaschaMann reviewed Jul 13, 2021

View reviewed changes

kwargs.jl Outdated Show resolved Hide resolved

Update kwargs.jl

8442719

Co-authored-by: Sascha Mann <git@mail.saschamann.eu>

SaschaMann reviewed Jul 18, 2021

View reviewed changes

DilumAluthge removed request for christopher-dG and davidanthoff July 18, 2021 08:29

handle the default state and improve error message

8603d27

c42f approved these changes Aug 20, 2021

View reviewed changes

kwargs.jl Outdated Show resolved Hide resolved

c42f mentioned this pull request Aug 20, 2021

Ensure broadcasted functions retain their bounds checks andyferris/Dictionaries.jl#64

Merged

Update kwargs.jl

6c2b827

Co-authored-by: Chris Foster <chris42f@gmail.com>

IanButterworth commented Dec 15, 2021

View reviewed changes

kwargs.jl Outdated Show resolved Hide resolved

Update kwargs.jl

f281faf

DilumAluthge requested a review from SaschaMann December 16, 2021 04:07

SaschaMann approved these changes Dec 22, 2021

View reviewed changes

SaschaMann merged commit 161c97c into julia-actions:master Dec 22, 2021

IanButterworth deleted the ib/bounds_check_control branch December 22, 2021 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow control of --check-bounds command option #46

Allow control of --check-bounds command option #46

IanButterworth commented Jul 6, 2021 •

edited

Loading

IanButterworth commented Jul 6, 2021

codecov-commenter commented Jul 6, 2021 •

edited

Loading

IanButterworth commented Jul 6, 2021

DilumAluthge commented Jul 6, 2021

SaschaMann commented Jul 6, 2021

SaschaMann left a comment

IanButterworth commented Jul 8, 2021

SaschaMann left a comment

IanButterworth commented Jul 12, 2021

SaschaMann commented Jul 13, 2021

IanButterworth commented Jul 13, 2021 •

edited

Loading

IanButterworth commented Jul 17, 2021

SaschaMann Jul 18, 2021 •

edited

Loading

IanButterworth Jul 20, 2021

SaschaMann commented Aug 11, 2021

c42f left a comment

IanButterworth commented Dec 14, 2021

SaschaMann commented Dec 22, 2021

Allow control of --check-bounds command option #46

Allow control of --check-bounds command option #46

Conversation

IanButterworth commented Jul 6, 2021 • edited Loading

IanButterworth commented Jul 6, 2021

codecov-commenter commented Jul 6, 2021 • edited Loading

Codecov Report

IanButterworth commented Jul 6, 2021

DilumAluthge commented Jul 6, 2021

SaschaMann commented Jul 6, 2021

SaschaMann left a comment

Choose a reason for hiding this comment

IanButterworth commented Jul 8, 2021

SaschaMann left a comment

Choose a reason for hiding this comment

IanButterworth commented Jul 12, 2021

SaschaMann commented Jul 13, 2021

IanButterworth commented Jul 13, 2021 • edited Loading

IanButterworth commented Jul 17, 2021

SaschaMann Jul 18, 2021 • edited Loading

Choose a reason for hiding this comment

IanButterworth Jul 20, 2021

Choose a reason for hiding this comment

SaschaMann commented Aug 11, 2021

c42f left a comment

Choose a reason for hiding this comment

IanButterworth commented Dec 14, 2021

SaschaMann commented Dec 22, 2021

IanButterworth commented Jul 6, 2021 •

edited

Loading

codecov-commenter commented Jul 6, 2021 •

edited

Loading

IanButterworth commented Jul 13, 2021 •

edited

Loading

SaschaMann Jul 18, 2021 •

edited

Loading