Skip to content

Conversation

@alonre24
Copy link
Collaborator

@alonre24 alonre24 commented Jun 6, 2022

Remove git-lfs from this project, and use S3 to store tests data (which includes some heavy models). We use s3://dev.cto.redis bucket, and restore data from it in CI using sync command. The data is also stored in circle-CI local cache

@alonre24 alonre24 requested review from DvirDukhan and chayim June 7, 2022 08:51
@alonre24 alonre24 marked this pull request as ready for review June 7, 2022 08:52
@codecov
Copy link

codecov bot commented Jun 7, 2022

Codecov Report

Merging #925 (81d4e1f) into master (2dd4ef1) will increase coverage by 0.44%.
The diff coverage is 96.20%.

@@            Coverage Diff             @@
##           master     #925      +/-   ##
==========================================
+ Coverage   81.30%   81.75%   +0.44%     
==========================================
  Files          54       54              
  Lines        8201     8220      +19     
==========================================
+ Hits         6668     6720      +52     
+ Misses       1533     1500      -33     
Impacted Files Coverage Δ
src/execution/DAG/dag_builder.c 100.00% <ø> (ø)
src/execution/DAG/dag_op.c 100.00% <ø> (ø)
src/execution/parsing/script_commands_parser.c 78.57% <ø> (-0.12%) ⬇️
.../serialization/RDB/decoder/previous/v2/decode_v2.c 46.37% <50.00%> (+0.45%) ⬆️
tests/module/LLAPI.c 75.89% <80.00%> (-0.02%) ⬇️
src/execution/background_workers.c 94.63% <95.23%> (+0.01%) ⬆️
src/redisai.c 89.08% <96.77%> (+1.29%) ⬆️
...backends/libtorch_c/torch_extensions/torch_redis.h 90.90% <100.00%> (ø)
src/backends/tensorflow.c 72.43% <100.00%> (+0.06%) ⬆️
src/execution/DAG/dag.c 91.50% <100.00%> (-0.28%) ⬇️
... 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 eca43e1...81d4e1f. Read the comment docs.

@alonre24 alonre24 added ci-test and removed ci-test labels Jun 7, 2022
@alonre24 alonre24 added ci-test and removed ci-test labels Jun 7, 2022
@alonre24 alonre24 added ci-test and removed ci-test labels Jun 7, 2022
@alonre24 alonre24 added ci-test and removed ci-test labels Jun 7, 2022
@alonre24 alonre24 added ci-test and removed ci-test labels Jun 7, 2022
- v1.2.5-deps-{{ checksum "get_deps.sh" }}-gpu
- restore_cache:
keys:
- v1.2-tests_data-gpu

Choose a reason for hiding this comment

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

what is the reason we are having an additional cache key? Should be the same for both modes, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, that's how it was before, but I got access denied when restoring cache, so I tried this option instead....


#----------------------------------------------------------------------------------------------

get_tests_data() {

Choose a reason for hiding this comment

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

I don't mind having this right now, but it forces developers to have aws account to run this command.
Please see https://stackoverflow.com/questions/273743/using-wget-to-recursively-fetch-a-directory-with-arbitrary-files-in-it if we can use wget with aws HTTP.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. Using recursive wget was not possible, so we got the desired behavior by using a file that specify all the relevant resources to download.

@alonre24 alonre24 added ci-test and removed ci-test labels Jun 8, 2022
@alonre24 alonre24 merged commit 5e4517d into master Jun 9, 2022
@alonre24 alonre24 deleted the move_test_data_to_s3 branch June 9, 2022 09:46
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