CI: fix subdirectory path globbing by anzz1 · Pull Request #546 · ggml-org/llama.cpp

anzz1 · 2023-03-27T04:53:48Z

CI: Fix subdirectory path globbing
CI: Fix Github runner AVX512F detection (Windows)

j-f1 · 2023-03-27T15:08:00Z

IMO it would be reasonable to drop the paths and just always run CI, what do you think?

anzz1 · 2023-03-27T15:25:52Z

IMO it would be reasonable to drop the paths and just always run CI, what do you think?
Nah, it's definitely better this way. It's been working fine so far and will continue to work, just that change to include subdirectories too will restore it to working order.

It simply doesn't make any sense to run CI builds on every readme file change. Tbh someone should the fix the docker too that it only builds when it needs to. Before the docker was in the ci the PR's were very clean since the non-code changes didn't go through the check rigamarole.

You could see at a glance without going inside the PR itself that if it doesnt have the ✅ / ❌ marks that whether it's a code change or not. Also the whole "approve and run" thing didn't exist for readme changes. Unfortunately I'm not familiar enough with docker that I could confidently fix it.

Also there are security implications for having the workflow paths and push/pr types exactly like it currently is, it's not a random decision. The way I have set it up is so that I can condifently say that I've secured the logic reasonably well.

You do say reasonable, but I cannot think of any good reasons for it. Like I can think of many reasons more besides the ones already said why it's better to have the CI mindfully set up than having a . "just do everything everytime always lol" type of thing.

But I can't think of a single reason why the "do everything always check nothing" CI build type would be better. I'm open to be convinced otherwise though, as maybe there is something I don't see or forget.

To me , you are suggesting to go backwards undoing the good CI setup because docker is already broken while the way I see it the right way would be to fix the docker build to have it make sense too and restore the clean CI style.

Even if it weren't fixed, theres no reason to break this one too.

j-f1 · 2023-03-27T15:50:11Z

But I can't think of a single reason why the "do everything always check nothing" CI build type would be better

My rationale was that if the checks are always run, we can never miss a change that breaks CI. We’re not paying for the compute time to run the tests, so seeing the ✅ indicates that nothing broke, even if the change is just a readme change.

In terms of security, what are you worried about? As far as I can tell, nothing uses pull_request_target, fork PRs are not given any more access than they should have, and people with commit access to this repo can just do malicious things directly instead of having to use an action.

anzz1 · 2023-03-27T17:09:18Z

My rationale was that if the checks are always run, we can never miss a change that breaks CI. We’re not paying for the compute time to run the tests, so seeing the white_check_mark indicates that nothing broke, even if the change is just a readme change.

Ah I see, thats a valid point if the CI was misconfigured. However, when properly configured (meaning that anything that can affect CI is explicitly defined), there simply aren't any changes to break CI to do. Readme changes can't break CI.

In terms of security, what are you worried about? As far as I can tell, nothing uses pull_request_target, fork PRs are not given any more access than they should have, and people with commit access to this repo can just do malicious things directly instead of having to use an action.

There are ways to cause mischief with the GH actions which I'm not going to go into more detail. You can "just trust me bro" or do your own research.

It is also simply good practice to limit the scope of the CI actions and be mindful when configuring them. Various guides about working with actions go into more detail about this.

Yes, we are not paying for the compute but there are also the Usage Limits to consider. And yes, we are not currently hitting any limits but I don't see why a code PR in the whisper.cpp should wait for a runner slot because maximum concurrent jobs are being reached because some readme changes were done in this repo at the same time. And even if we had all the unlimited compute in the world, I still don't get why argue for wasting energy even if it costs nothing to you.

I don't even understand why we are arguing about this? I can understand that there are times when doing something the "smart way" isn't worth the effort. But especially as the work to do it the "smart way" has already been put in, I don't get why it should be undone to do it the "dumb way".

Can we just get over this already and move on to better things? This is a dumb thing to have an argument about, we both have better things to do. Please 😸

Green-Sky · 2023-03-27T17:31:43Z

.github/workflows/build.yml

  pull_request:
    types: [opened, synchronize, edited, reopened, review_requested, ready_for_review]
-    paths: ['CMakeLists.txt', 'Makefile', '**.h', '*.c', '**.cpp']
+    paths: ['**/CMakeLists.txt', '**/Makefile', '**/*.h', '**/*.c', '**/*.cpp']


someone explain to me why they look the way they do.

It's the glob format

In plain english it says:
in all directories and subdirectories look for these:
CMakeLists.txt
Makefile
*.h
*.c
*.cpp

and you cant just use ** instead of **/* ? that feels kind of wrong

and you cant just use ** instead of **/* ? that feels kind of wrong

Yeah, for example **/Makefile matches files named exactly Makefile , while **Makefile would also match abcdefgMakefile and so on. The **/* format is the proper format for recursing directories.

j-f1 · 2023-03-27T23:28:52Z

Can we just get over this already and move on to better things? This is a dumb thing to have an argument about, we both have better things to do. Please 😸

Sure, I will not object to merging this PR. You make some good points!

xor ecx,ecx

ggerganov · 2023-03-28T18:10:35Z

.github/workflows/build.yml

        run: |
          cd build
-          Set-Content -Path .\avx512f.exe -Value ([Convert]::FromBase64String('TVqQAAMAAAAEAAAA//8AALgAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAyAAAAA4fug4AtAnNIbgBTM0hVGhpcyBwcm9ncmFtIGNhbm5vdCBiZSBydW4gaW4gRE9TIG1vZGUuDQ0KJAAAAAAAAAClmfXY4fibi+H4m4vh+JuL4fiai+P4m4si98aL4vibi7Xbq4vg+JuLUmljaOH4m4sAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABQRQAATAEBAGo6H2QAAAAAAAAAAOAADwELAQYAAAIAAAAAAAAAAAAADBAAAAAQAAAAIAAAAABAAAAQAAAAAgAABAAAAAAAAAAEAAAAAAAAAAAgAAAAAgAAAAAAAAMAAAAAABAAABAAAAAAEAAAEAAAAAAAABAAAAAAAAAAAAAAAFQQAAAoAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAAADAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAC50ZXh0AAAAsgAAAAAQAAAAAgAAAAIAAAAAAAAAAAAAAAAAACAAAGAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACUEAAAiBAAAAAAAABVi+xRUVNTuAcAAAAPosHrEGaD4wGJXfxbg0X8MI1F+GoAUI1F/GoBUGr1/xUAEEAAUP8VBBBAAItF/FuDwND32BvAQMnDzMx8EAAAAAAAAAAAAACkEAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAlBAAAIgQAAAAAAAApANXcml0ZUZpbGUAuQFHZXRTdGRIYW5kbGUAAEtFUk5FTDMyLmRsbAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA==')) -AsByteStream
+          Set-Content -Path .\avx512f.exe -Value ([Convert]::FromBase64String('TVqQAAMAAAAEAAAA//8AALgAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAyAAAAA4fug4AtAnNIbgBTM0hVGhpcyBwcm9ncmFtIGNhbm5vdCBiZSBydW4gaW4gRE9TIG1vZGUuDQ0KJAAAAAAAAAClmfXY4fibi+H4m4vh+JuL4fiai+P4m4si98aL4vibi7Xbq4vg+JuLUmljaOH4m4sAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABQRQAATAEBAGmkImQAAAAAAAAAAOAADwELAQYAAAIAAAAAAAAAAAAADBAAAAAQAAAAIAAAAABAAAAQAAAAAgAABAAAAAAAAAAEAAAAAAAAAAAgAAAAAgAAAAAAAAMAAAAAABAAABAAAAAAEAAAEAAAAAAAABAAAAAAAAAAAAAAAFQQAAAoAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAAADAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAC50ZXh0AAAAsgAAAAAQAAAAAgAAAAIAAAAAAAAAAAAAAAAAACAAAGAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACUEAAAiBAAAAAAAABVi+xRUVNTM8m4BwAAAA+iwesQZoPjAYld/FuDRfwwjUX4agBQjUX8agFQavX/FQAQQABQ/xUEEEAAi0X8W4PA0PfYG8BAycN8EAAAAAAAAAAAAACkEAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAlBAAAIgQAAAAAAAApANXcml0ZUZpbGUAuQFHZXRTdGRIYW5kbGUAAEtFUk5FTDMyLmRsbAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA==')) -AsByteStream


@anzz1

I saw your opinion about the AV software thinking this is a virus and I partially agree that it is stupid.
But still, I think we have to remove this.

We can add a small C program check-avx512f that simply calls a AVX512 instruction and if it crashes we know it is not supported. The program will be compiled by the CI and this way we won't need to hardcode binary executables like this

The AV issue in question was flagging the test-tokenizer in the compiled release, not this, the .yml being a text file isn't even executable.

But yeah I do agree that while kinda elegant, the solution is also pretty obtuse. I did think to make a new github action for checking proc features, but the process of it is kinda hassle and is very javascript-heavy so didn't bother.

I'll remove that for now if that's what wanted and think of a better solution. Compiling a C program could be a good bet. I mean it is exactly that, a compiled C program but having it in code than a binary form would be nice I guess. Unfortunately such easy cat /proc/cpuinfo doesn't exist on windows.

edit: ah i get it now, the 'detection fix' does not reference the AV issue, it references detection of AVX512, which it didnt do :D i made an error with the code reading the intel ref manual and missed the part of needing to clear the ecx reg when calling that specific leaf for cpu info

New implemenation, replacing opacity with verbosity #584

* iq2_kt and iq3_kt work with new int trellis Much slower than the fp16 based trellis. I guess, Apple doesn't have int8_t SIMD on the M2-Max GPU. * q4_0 83.6 t/s -> 128.4 t/s. q4_0_r8 is at 123.5 t/s * q5_0 74.2 t/s -> 128.5 t/s. q5_0_r4 is at 111.4 t/s. * q6_0 74.2 t/s -> 128.8 t/s. q6_0_r4 is at 107.2 t/s. * q8_0 84.5 -> 128.7 t/s. q8_0_r8 is at 131 t/s. * iq4_nl 84.5 t/s -> 128.1 t/s. iq4_nl_r4 is at 120.4 t/s * q4_1 74.4 -> 115.4 t/s. There is no repacked variant * q5_1 64.2 t/s -> 114.9 t/s. There is no repacked variant. --------- Co-authored-by: Iwan Kawrakow <iwan.kawrakow@gmail.com>

CI: fix subdirectory path globbing

1bf20a3

anzz1 requested review from Green-Sky, blackhole89, ggerganov, slaren and sw March 27, 2023 05:25

anzz1 added bug Something isn't working build Compilation issues labels Mar 27, 2023

anzz1 requested review from antimatter15, bernatvadell, cocktailpeanut, eiz, gjmulder, prusnak, rgerganov and thement March 27, 2023 07:59

anzz1 added the high priority Very important issue label Mar 27, 2023

Green-Sky reviewed Mar 27, 2023

View reviewed changes

anzz1 mentioned this pull request Mar 27, 2023

Enable Fused-Multiply-Add (FMA) and F16C/CVT16 vector extensions on MSVC #375

Merged

CI: github runner avx512f detection fix (windows)

99590bf

xor ecx,ecx

anzz1 requested a review from Green-Sky March 28, 2023 14:56

ggerganov requested changes Mar 28, 2023

View reviewed changes

Update build.yml

8765be5

anzz1 requested a review from ggerganov March 28, 2023 19:07

ggerganov approved these changes Mar 28, 2023

View reviewed changes

anzz1 merged commit f121705 into master Mar 28, 2023

anzz1 deleted the patch-fix-ci-subdir-glob branch March 28, 2023 19:43

Bearsaerker mentioned this pull request Mar 12, 2025

Eval bug: Gemma 3 extremly slow prompt processing when using quantized kv cache. #12352

Closed

Conversation

anzz1 commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

j-f1 commented Mar 27, 2023

Uh oh!

anzz1 commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

j-f1 commented Mar 27, 2023

Uh oh!

anzz1 commented Mar 27, 2023

Uh oh!

Green-Sky Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

anzz1 Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

anzz1 Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Green-Sky Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

anzz1 Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

j-f1 commented Mar 27, 2023

Uh oh!

ggerganov Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

anzz1 Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anzz1 Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

anzz1 commented Mar 27, 2023 •

edited

Loading

anzz1 commented Mar 27, 2023 •

edited

Loading

anzz1 Mar 27, 2023 •

edited

Loading

anzz1 Mar 28, 2023 •

edited

Loading