Remove default CPU_TARGET=avx #10346

majetideepak · 2024-06-28T14:33:31Z

The default should be empty so that the CPU_TARGET/CPU_ARCH will be inferred.
Remove SSE support for macos as developers are mostly on Intel avx or Apple arm.

netlify · 2024-06-28T14:33:47Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`004468d`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/668e8527b0928c0008eccb3f

czentgr

Looks good!

scripts/setup-adapters.sh

czentgr · 2024-07-03T14:37:41Z

scripts/setup-helper-functions.sh

  case $CPU_ARCH in

    "arm64")
-      echo -n "-mcpu=apple-m1+crc -std=c++17 -fvisibility=hidden $ADDITIONAL_FLAGS"
+      echo -n "-mcpu=apple-m1+crc -std=c++17 -fvisibility=hidden"


Do we not need -fvisibility-inlines-hidden being added here? It was previously present to avoid log flooding.

I don't see the log messages after removing that. We need to evaluate and specify -fvisibility=hidden and -fvisibility-inlines-hidden for both clang and gcc.

kgpai

My one concern with this change is that we cannot force builds to use a particular architecture (for e.g you want an SSE only build) - This is sometimes required for e.g if you want to create portable binaries that may need to run on machines that dont have avx.

kgpai · 2024-07-08T21:09:48Z

.github/workflows/docker.yml

@@ -46,16 +46,14 @@ jobs:
        include:
          - name: Check
            file: "scripts/check-container.dockfile"
-            args: "cpu_target=avx"
            tags: "ghcr.io/facebookincubator/velox-dev:check-avx"


Should we rename the tag to check from check-avx then ?

kgpai · 2024-07-08T21:14:27Z

scripts/setup-helper-functions.sh

-        if [[ $CPU_CAPABILITIES =~ "avx" ]]; then
+  if [ "$OS" = "Darwin" ]; then
+    if [ "$MACHINE" = "arm64" ]; then
+      CPU_ARCH="arm64"


I suppose that mac's with cpu's that dont support avx are not supported anymore ?

Mac is used by developers and most of them are either on newer ARM64 or the older x86_64 with avx support.
Avx in both Intel and AMD was first available in 2011. We should be safe with the assumption that avx is always available for macs with x86_64.
https://en.wikipedia.org/wiki/Advanced_Vector_Extensions

Sounds fair - just wanted to confirm.

kgpai · 2024-07-08T21:16:36Z

scripts/ubuntu-22.04-cpp.dockerfile

@@ -32,6 +29,8 @@ ADD scripts /velox/scripts/
 # are required to avoid tzdata installation
 # to prompt for region selection.
 ARG DEBIAN_FRONTEND="noninteractive"
+# Set a default timezone, can be overriden via ARG
+ARG tz="Europe/Madrid"


I wonder why we set Europe/Madrid in first place ..

Jacob added this file and he set Europe as the default.

nit: I think its safe to remove this or maybe set it to UTC.

We cannot remove the default arg. Ubuntu tzdata requires this as mentioned in the comment above.
I changed the default to Etc/UTC.
Reference https://serverfault.com/questions/949991/how-to-install-tzdata-on-a-ubuntu-docker-image

majetideepak · 2024-07-09T14:44:11Z

My one concern with this change is that we cannot force builds to use a particular architecture (for e.g you want an SSE only build

@kgpai as mentioned in the other comment, avx has been available since a long time now. I doubt there is a use case to build only for sse.

majetideepak · 2024-07-09T15:02:31Z

We can always add the CPU_TARGET back if there is a use for it.

kgpai · 2024-07-09T16:42:31Z

@majetideepak Should we also change the documentation as part of this change (The README still refers to this )

kgpai

LGTM (one minor nit though).

facebook-github-bot · 2024-07-10T17:24:07Z

@pedroerp has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-07-10T18:33:10Z

@pedroerp merged this pull request in 7f2d7ad.

conbench-facebook · 2024-07-10T18:59:25Z

Conbench analyzed the 1 benchmark run on commit 7f2d7ada.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Summary: Image name was changed as part of #10346 Pull Request resolved: #10438 Reviewed By: pedroerp Differential Revision: D59776586 Pulled By: mbasmanova fbshipit-source-id: b233f8a981636f4a09d6cc79eec80fd1b76142d0

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 28, 2024

This was referenced Jul 1, 2024

Add ARM64 Support for Building Prestissimo Docker Image on Mac M1 prestodb/presto#23009

Open

Docker compose fails with 'Failed to run install_mvfst .' #10291

Open

majetideepak force-pushed the fix-docker branch 2 times, most recently from dfb145f to 879be49 Compare July 2, 2024 06:05

majetideepak marked this pull request as ready for review July 2, 2024 06:05

majetideepak requested review from assignUser, kgpai and czentgr July 2, 2024 06:07

czentgr reviewed Jul 2, 2024

View reviewed changes

scripts/setup-adapters.sh Outdated Show resolved Hide resolved

majetideepak force-pushed the fix-docker branch from 2fca522 to ea11212 Compare July 2, 2024 12:48

czentgr reviewed Jul 3, 2024

View reviewed changes

kgpai reviewed Jul 8, 2024

View reviewed changes

majetideepak force-pushed the fix-docker branch from 1df12f9 to 3f370bb Compare July 9, 2024 14:36

majetideepak added 2 commits July 9, 2024 10:36

Remove CPU_TARGET and automate arch inference

9c56a41

add exit and address review comments

3f370bb

majetideepak changed the title ~~Remove CPU_TARGET and automate arch inference~~ Remove default CPU_TARGET=avx Jul 9, 2024

majetideepak force-pushed the fix-docker branch from ba69db6 to 9d40159 Compare July 9, 2024 17:52

keep CPU_TARGET in setup-helper-functions.sh

9d40159

kgpai approved these changes Jul 9, 2024

View reviewed changes

kgpai added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Jul 9, 2024

change default ubuntu tz to Etc/UTC

ce9ca68

czentgr approved these changes Jul 9, 2024

View reviewed changes

add default avx for darwin

004468d

majetideepak force-pushed the fix-docker branch from ed2483a to 004468d Compare July 10, 2024 12:57

facebook-github-bot closed this in 7f2d7ad Jul 10, 2024

facebook-github-bot added the Merged label Jul 10, 2024

majetideepak mentioned this pull request Jul 10, 2024

Fix CI Image name for check #10438

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove default CPU_TARGET=avx #10346

Remove default CPU_TARGET=avx #10346

majetideepak commented Jun 28, 2024 •

edited

Loading

netlify bot commented Jun 28, 2024 •

edited

Loading

czentgr left a comment

czentgr Jul 3, 2024

majetideepak Jul 8, 2024

kgpai left a comment

kgpai Jul 8, 2024

majetideepak Jul 9, 2024

kgpai Jul 8, 2024

majetideepak Jul 9, 2024

kgpai Jul 9, 2024

kgpai Jul 8, 2024

majetideepak Jul 9, 2024

kgpai Jul 9, 2024

majetideepak Jul 9, 2024

majetideepak commented Jul 9, 2024 •

edited

Loading

majetideepak commented Jul 9, 2024

kgpai commented Jul 9, 2024

kgpai left a comment

facebook-github-bot commented Jul 10, 2024

facebook-github-bot commented Jul 10, 2024

conbench-facebook bot commented Jul 10, 2024

Remove default CPU_TARGET=avx #10346

Remove default CPU_TARGET=avx #10346

Conversation

majetideepak commented Jun 28, 2024 • edited Loading

netlify bot commented Jun 28, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

czentgr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgpai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majetideepak commented Jul 9, 2024 • edited Loading

majetideepak commented Jul 9, 2024

kgpai commented Jul 9, 2024

kgpai left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 10, 2024

facebook-github-bot commented Jul 10, 2024

conbench-facebook bot commented Jul 10, 2024

majetideepak commented Jun 28, 2024 •

edited

Loading

netlify bot commented Jun 28, 2024 •

edited

Loading

majetideepak commented Jul 9, 2024 •

edited

Loading