artificialwisdomai / origin

Artificial Wisdom™ Cloud Platform
Apache License 2.0
2 stars 4 forks source link

Improve faiss by using updated MKL #130

Closed sdake closed 8 months ago

sdake commented 8 months ago

@MostAwesomeDude thanks! I actually pushed htis up last night when I hit the wall so to speak. I have a version that builds now.

The real goal with these changes is to achieve semver+epoch, ie: 1.7.4+0.

rstarmer commented 8 months ago

Just curious, for the Dockerfile, is there a specific reason that you broke out every package into its own layer (e.g. RUN apt install X; RUN apt install Y;...)

Robert

On Mon, Dec 4, 2023 at 2:55 PM Corbin Simpson @.***> wrote:

@.**** approved this pull request.

I'm still waiting for step 58 to complete (compiling FAISS) but this looks likely to succeed. Thanks for improving the readability with those long option names!

— Reply to this email directly, view it on GitHub https://github.com/artificialwisdomai/origin/pull/130#pullrequestreview-1763592148, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHDOZR3X7ML6VWCRGAKSUDYHZIGJAVCNFSM6AAAAABAFHKR62VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONRTGU4TEMJUHA . You are receiving this because your review was requested.Message ID: @.***>

sdake commented 8 months ago

Just curious, for the Dockerfile, is there a specific reason that you broke out every package into its own layer (e.g. RUN apt install X; RUN apt install Y;...) Robert On Mon, Dec 4, 2023 at 2:55 PM Corbin Simpson @.> wrote: @*.*** approved this pull request. I'm still waiting for step 58 to complete (compiling FAISS) but this looks likely to succeed. Thanks for improving the readability with those long option names! — Reply to this email directly, view it on GitHub <#130 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHDOZR3X7ML6VWCRGAKSUDYHZIGJAVCNFSM6AAAAABAFHKR62VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONRTGU4TEMJUHA . You are receiving this because your review was requested.Message ID: @.>

There are many approaches to installing packaged tools within Linux.

The two obvious alternatives: B. RUN apt -y install build-essential python3 python3-full python3-venv python3-pip swig ninja-build git cmake gpg curl zstd C. RUN apt -y install build-essential && apt -y install python3 && apt -y install python3-full && apt install python3-venv && p apt -y install python3-pip && apt -y install swig && apt -y install ninja-build && apt-install git && apt -uy install cmake && apt -y install gpg && apt -y curl && apt -y zstd

A from this PR:

RUN apt install -y build-essential
RUN apt install -y python3
RUN apt install -y python3-full
RUN apt install -y python3-venv
RUN apt install -y python3-pip
RUN apt install -y swig
RUN apt install -y ninja-build
RUN apt install -y git
RUN apt install -y cmake
RUN apt install -y gpg
RUN apt install -y curl
RUN apt install -y zstd

Can you take a look at the approaches, A, B, C and define which one offers the best software quality?

I would encourage ignoring layers, because, as you can see, this PR only spits out a binary in the host filesystem in target/.... If you wish to evaluate with layers, can you tell me why layer minimization matters?

Thanks -steve

sdake commented 8 months ago

@rstarmer @MostAwesomeDude this latest patch concludes this patch series. The goal of this patch series was to ensure an updated faiss is installed, rather than a downrev version which happens today.