Skip to content

remove submodule requirement for dependency fetching #786

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 22, 2021

Conversation

chayim
Copy link
Contributor

@chayim chayim commented Jun 16, 2021

No description provided.

@chayim chayim requested a review from DvirDukhan June 16, 2021 09:07
@codecov
Copy link

codecov bot commented Jun 16, 2021

Codecov Report

Merging #786 (8f875f1) into master (dfc0963) will increase coverage by 0.70%.
The diff coverage is 73.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #786      +/-   ##
==========================================
+ Coverage   79.22%   79.92%   +0.70%     
==========================================
  Files          50       52       +2     
  Lines        7662     7767     +105     
==========================================
+ Hits         6070     6208     +138     
+ Misses       1592     1559      -33     
Impacted Files Coverage Δ
src/backends/libtflite_c/tflite_c.cpp 57.60% <0.00%> (-2.50%) ⬇️
src/backends/libtorch_c/torch_c.cpp 62.26% <0.00%> (-1.18%) ⬇️
src/backends/tensorflow.c 69.04% <0.00%> (-1.55%) ⬇️
src/backends/tflite.c 66.01% <0.00%> (ø)
src/backends/torch.c 81.71% <0.00%> (ø)
src/execution/utils.c 67.85% <ø> (ø)
src/serialization/AOF/rai_aof_rewrite.c 0.00% <0.00%> (ø)
src/backends/backends.c 75.36% <55.96%> (+12.20%) ⬆️
src/execution/run_queue_info.c 65.90% <65.90%> (ø)
src/execution/parsing/deprecated.c 81.86% <71.42%> (+1.19%) ⬆️
... and 16 more

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 1fdc22b...8f875f1. Read the comment docs.

@chayim chayim added the ci-test label Jun 17, 2021
@DvirDukhan
Copy link
Collaborator

@Spartee please add your review

@Spartee
Copy link
Contributor

Spartee commented Jun 17, 2021

I could not get this to work on MacOS

Steps I followed

  1. git clone https://github.com/RedisAI/RedisAI.git --branch ck-optional-readies-depsonly --depth=1

which seems to still download (or at least filter?) the 400Mb of git-lfs content

❯ git clone https://github.com/RedisAI/RedisAI.git --branch ck-optional-readies-depsonly --depth=1
Cloning into 'RedisAI'...
remote: Enumerating objects: 346, done.
remote: Counting objects: 100% (346/346), done.
remote: Compressing objects: 100% (321/321), done.
remote: Total 346 (delta 38), reused 194 (delta 9), pack-reused 0
Receiving objects: 100% (346/346), 4.50 MiB | 24.14 MiB/s, done.
Resolving deltas: 100% (38/38), done.
Filtering content: 100% (30/30), 395.78 MiB | 36.30 MiB/s, done.
  1. ./get_deps.sh

which gives

x86_64
Cloning dlpack ...
Cloning into 'dlpack'...
remote: Enumerating objects: 36, done.
remote: Counting objects: 100% (36/36), done.
remote: Compressing objects: 100% (26/26), done.
remote: Total 36 (delta 0), reused 23 (delta 0), pack-reused 0
Unpacking objects: 100% (36/36), done.
Done.
Installing TensorFlow ...
There are errors.

the verbose output from VERBOSE=1 ./get_deps.sh

❯ VERBOSE=1 ./get_deps.sh
+ [[ '' == \c\p\u ]]
+ [[ '' == 1 ]]
+ [[ '' == \g\p\u ]]
+ [[ '' == 1 ]]
+ GPU=0
+ [[ 0 == 1 ]]
+ DEVICE=cpu
+ '[' -f /Users/spartee/scratch/SmartSim/smartsim/.third-party/RedisAI/opt/readies/bin/platform ']'
++ uname -s
++ tr '[:upper:]' '[:lower:]'
+ OS=darwin
+ uname -m
+ grep aarch64
+ ARCH=x64
+ uname -m
+ grep x86
x86_64
+ [[ darwin == macos ]]
+ DEPS_DIR=/Users/spartee/scratch/SmartSim/smartsim/.third-party/RedisAI/deps/darwin-x64-cpu
+ mkdir -p /Users/spartee/scratch/SmartSim/smartsim/.third-party/RedisAI/deps/darwin-x64-cpu
+ cd /Users/spartee/scratch/SmartSim/smartsim/.third-party/RedisAI/deps/darwin-x64-cpu
+ DLPACK=dlpack
+ LIBTENSORFLOW=libtensorflow
+ LIBTFLITE=libtensorflow-lite
+ LIBTORCH=libtorch
+ MKL=mkl
+ ONNXRUNTIME=onnxruntime
+ DLPACK_VERSION=v0.5_RAI
+ [[ '' != 0 ]]
+ [[ '' == 1 ]]
+ [[ ! -d dlpack ]]
+ echo 'dlpack is in place.'
dlpack is in place.
+ TF_VERSION=2.4.0
+ [[ '' != 0 ]]
+ [[ '' == 1 ]]
+ [[ ! -d libtensorflow ]]
+ echo 'Installing TensorFlow ...'
Installing TensorFlow ...
+ [[ darwin == linux ]]
+ [[ darwin == macos ]]
+ LIBTF_ARCHIVE=libtensorflow----2.4.0.tar.gz
+ [[ ! -f libtensorflow----2.4.0.tar.gz ]]
+ wget -q /libtensorflow----2.4.0.tar.gz
++ error
++ echo 'There are errors.'
There are errors.
++ exit 1

If I was meant to still include the --recursive, I tried that and I think that will work but still running into the ONNX on MacOS bug with the bad link which I think is documented in #743

I can try this on other systems, but am I doing something wrong? Am I still supposed to use --recursive? isn't the point of this PR to get rid of the submodules? apologies if I have missed something.

@chayim
Copy link
Contributor Author

chayim commented Jun 21, 2021

  1. git clone https://github.com/RedisAI/RedisAI.git --branch ck-optional-readies-depsonly --depth=1

@Spartee Is this solved by the GIT_LFS_SKIP_SMUDGE=1 change that we discussed? When running GIT_LFS_SKIP_SMUDGE=1 git clone https://github.com/RedisAI/RedisAI.git --branch ck-optional-readies-depsonly --depth=1 I receive the following output:

Cloning into 'RedisAI'...
remote: Enumerating objects: 346, done.
remote: Counting objects: 100% (346/346), done.
remote: Compressing objects: 100% (321/321), done.
remote: Total 346 (delta 38), reused 194 (delta 9), pack-reused 0
Receiving objects: 100% (346/346), 4.50 MiB | 4.36 MiB/s, done.
Resolving deltas: 100% (38/38), done.

Given that delta, I think this can now merge. The point of the PR isn't to remove submodules - only to disable updating those submodules as part of our get_deps run.

@chayim chayim merged commit 672edbe into master Jun 22, 2021
@chayim chayim deleted the ck-optional-readies-depsonly branch June 22, 2021 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants