Skip to content

Comments

CI: use macOS ARM-based runner#3395

Merged
nilason merged 1 commit intoOSGeo:mainfrom
nilason:test_apple_arm
Feb 15, 2024
Merged

CI: use macOS ARM-based runner#3395
nilason merged 1 commit intoOSGeo:mainfrom
nilason:test_apple_arm

Conversation

@nilason
Copy link
Contributor

@nilason nilason commented Feb 2, 2024

Replace the macOS runner with the new ARM64 based one.

Two tests are not passing, so I put them on the exclusion list.

This will cut the CI run time with approximately 50 %!

@nilason nilason added this to the 8.4.0 milestone Feb 2, 2024
@nilason nilason self-assigned this Feb 2, 2024
@github-actions github-actions bot added the CI Continuous integration label Feb 2, 2024
@petrasovaa
Copy link
Contributor

petrasovaa commented Feb 2, 2024

Two tests are not passing, so I put them on the exclusion list.

Is there a way to see why they are not passing, specifically I am wondering about r.horizon test.

@nilason
Copy link
Contributor Author

nilason commented Feb 2, 2024 •
edited
Loading

Two tests are not passing, so I put them on the exclusion list.

Is there a way to see why they are not passing, specifically I am wondering about r.horizon test.

I planned to create tickets for them, but to give you a head start:

Details 
test_r_horizon

./raster/r.horizon – test_r_horizon

❌ Test failed

Test	test_r_horizon
Testsuite	./raster/r.horizon
Test file	./raster/r.horizon/testsuite/test_r_horizon.py
Status	FAILED
Return code	1
Number of tests	6
Successful tests	5
Failed tests	1
Percent successful	83%
Test duration	0:00:21.037551
Tested modules	r.horizon
Supplementary files

standard output (stdout)
standard error output (stderr)
Standard error output (stderr)

.F....
======================================================================
FAIL: test_point_mode_multiple_direction_artificial (__main__.TestHorizon.test_point_mode_multiple_direction_artificial)
Test mode with 1 point and multiple directions with artificial surface
----------------------------------------------------------------------
Traceback (most recent call last):
  File "raster/r.horizon/testsuite/test_r_horizon.py", line 159, in test_point_mode_multiple_direction_artificial
    self.assertMultiLineEqual(first=ref4, second=stdout)
  File "etc/python/grass/gunittest/case.py", line 179, in assertMultiLineEqual
    return super().assertMultiLineEqual(first=first, second=second, msg=msg)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 'azim[15 chars]ght\n0.000000,0.197017\n20.000000,0.196832\n40[327 chars]63\n' != 'azim[15 chars]ght\n360.000000,0.197017\n20.000000,0.196832\n[329 chars]63\n'
  azimuth,horizon_height
- 0.000000,0.197017
+ 360.000000,0.197017
? ++
  20.000000,0.196832
  40.000000,0.196875
  60.000000,0.196689
  80.000000,0.196847
  100.000000,0.196645
  120.000000,0.196969
  140.000000,0.196778
  160.000000,0.196863
  180.000000,0.197017
  200.000000,0.196832
  220.000000,0.196875
  240.000000,0.196689
  260.000000,0.196847
  280.000000,0.196645
  300.000000,0.196969
  320.000000,0.196778
  340.000000,0.196863


----------------------------------------------------------------------
Ran 6 tests in 20.967s
FAILED (failures=1)

<\details>

@nilason
Copy link
Contributor Author

nilason commented Feb 2, 2024

For the record: building now takes about 2.5 min, the run in total some 33 min. Compared to ~7 min and ~79 min.

@echoix
Copy link
Member

echoix commented Feb 2, 2024

The extra core (3 core instead of 2), and the fact that they are fast, seems to help a lot! Even if our tests aren't really parallelized, it's still faster!

@nilason
Copy link
Contributor Author

nilason commented Feb 2, 2024

The extra core (3 core instead of 2), and the fact that they are fast, seems to help a lot! Even if our tests aren't really parallelized, it's still faster!

I was literally blown away, the first time I compiled GRASS with a M1 Pro (8-core). Compilation can take advantage of both parallelisation and the sheer speed of Apple's ARM architecture.

@echoix
Copy link
Member

echoix commented Feb 3, 2024

I'm kinda suprised that only two tests failed. Especially for the r3.flow, the numerical differences are quite strange. What do these two modules do differently (or correctly in tests) from the others? If it was only architectural differences, I would have expected way more.

@petrasovaa petrasovaa mentioned this pull request Feb 6, 2024
Merged
@echoix echoix mentioned this pull request Feb 8, 2024
Merged
echoix
echoix reviewed Feb 14, 2024
View reviewed changes
@nilason nilason added the macOS macOS specific label Feb 15, 2024
echoix
echoix approved these changes Feb 15, 2024
View reviewed changes
Copy link
Member

@echoix echoix left a comment

Choose a reason for hiding this comment

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

Let's try with this, it'll speed up the feedback loop a fair bit, and is closer to what macOS users would use now. Intel Macs are getting older and older

@nilason
Copy link
Contributor Author

nilason commented Feb 15, 2024

Let's try with this, it'll speed up the feedback loop a fair bit, and is closer to what macOS users would use now. Intel Macs are getting older and older

I agree. And the only "regression" on tests is reported with #3398 (which seems more like an "imperfect" test rather than actual bug).

@nilason nilason merged commit 05b9b03 into OSGeo:main Feb 15, 2024
@echoix
Copy link
Member

echoix commented Feb 15, 2024

Since the regression is already flagged elsewhere, I'm fine going forward with this. That's for your PR! You were the one that made me know about that new change, usually I read GitHub's changelog blog every couple of weeks, and didn't know about it when your PR landed.

jadenabrams100 pushed a commit to ncsu-csc472-spring2024/grass-CI-playground that referenced this pull request Feb 21, 2024
@nilason @jadenabrams100
CI: use macOS ARM-based runner (OSGeo#3395)
4732f8b
@cyliang368 cyliang368 mentioned this pull request Jun 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@echoix echoix echoix approved these changes

Assignees

@nilason nilason

Labels

CI Continuous integration macOS macOS specific

Projects

None yet

Milestone

8.4.0

Development

Successfully merging this pull request may close these issues.

3 participants

@nilason @petrasovaa @echoix