Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor: Rewrite data cp implementation #506

Merged
merged 48 commits into from
May 27, 2022
Merged

refactor: Rewrite data cp implementation #506

merged 48 commits into from
May 27, 2022

Conversation

andriihomiak
Copy link
Contributor

@andriihomiak andriihomiak commented Apr 29, 2022 •
edited
Loading

This PR addresses many issues with previous code for neuro-extras data cp command (#500,#301). Main idea of the proposed changes is to modularize key processes, related to data copy and add some useful abstractions to simplify further addition of supported sources/destinations (like neu.ro platform buckets or Google Drive #235).

Readiness checklist

  • Previous source/destination combinations are fully supported
    • Local-to-Local
      • Plain Copy
      • Extraction
      • Compression
      • Extraction + Compression
    • Local-to-Cloud
      • Plain Copy
      • Extraction
      • Compression
      • Extraction + Compression
    • Cloud-to-Local
      • Plain Copy
      • Extraction
      • Compression
      • Extraction + Compression
    • Platform-to-Cloud
      • Plain Copy
      • Extraction
      • Compression
      • Extraction + Compression
    • Cloud-to-Platform
      • Plain Copy
      • Extraction
      • Compression
      • Extraction + Compression
    • Platform-to-Platform (between storage: and disk: only)

      storage: to storage: and disk: to disk: are pretty much supported, but will be introduced in separate PR

      • Plain Copy
      • Extraction
      • Compression
      • Extraction + Compression
  • Tests
    • Conftests passing
    • E2E
      • Old passing
      • New (will be in a separate PR)
        • Exist
        • Passing
  • Docs
    • New code is documented
    • Changelog is added

@andriihomiak andriihomiak added data_ingestion Issue on `neuro-extras data cp` functionality python Pull requests that update Python code labels Apr 29, 2022
@andriihomiak andriihomiak self-assigned this Apr 29, 2022
@andriihomiak andriihomiak marked this pull request as ready for review May 10, 2022 11:03
YevheniiSemendiak
YevheniiSemendiak reviewed May 11, 2022
View reviewed changes
Copy link
Collaborator

@YevheniiSemendiak YevheniiSemendiak left a comment

Choose a reason for hiding this comment

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

Great job, thank you a lot for refactoring the spaghetti code we have written :)
All looks good to me, only some suggestions came to my mind, apply them or not - up to you.

neuro_extras/data/operations.py Outdated Show resolved Hide resolved
neuro_extras/data/operations.py Outdated Show resolved Hide resolved
CHANGELOG.D/506.misc Show resolved Hide resolved
neuro_extras/data/gcs.py Outdated Show resolved Hide resolved
neuro_extras/data/operations.py Show resolved Hide resolved
neuro_extras/data/operations.py Outdated Show resolved Hide resolved
neuro_extras/data/common.py Outdated Show resolved Hide resolved
neuro_extras/data/local.py Outdated Show resolved Hide resolved
neuro_extras/data/remote.py Outdated Show resolved Hide resolved
neuro_extras/data/archive.py Outdated Show resolved Hide resolved
YevheniiSemendiak
YevheniiSemendiak approved these changes May 13, 2022
View reviewed changes
Copy link
Collaborator

@YevheniiSemendiak YevheniiSemendiak left a comment

Choose a reason for hiding this comment

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

The code LGTM, great job, man!
Please, let's get the tests working before merging.

@andriihomiak
Copy link
Contributor Author

e2e tests are addressed in #515

@andriihomiak andriihomiak mentioned this pull request May 19, 2022
Closed
@andriihomiak andriihomiak linked an issue May 19, 2022 that may be closed by this pull request
[data_ingestion] Add tests on copying TO cloud #76
Closed
andriihomiak and others added 16 commits May 23, 2022 15:16
@andriihomiak
test: data cp to cloud (#515)
99a6fac 
* test: Refactor data cp tests

* tests: Enable passing of archive types via env variable

* test: Skip data copy tests on win32
@andriihomiak
test: Remove unnecesary retry
73b8562
@andriihomiak
test: Change number of workers
fd16a90
@andriihomiak
test: Skip cloud-to-local and local-to-cloud tests; increase timeout …
e974646 
…for tests
@andriihomiak
misc: Log only id of the job
d90d265
@andriihomiak
test: Increase test timeout to 90min
3d62bb0
@andriihomiak
feat: Use smaller default preset for data copy
f3a913f
@andriihomiak
test: Decrease paralellism
2ce07ce
@andriihomiak
ci: Limit jobs
f23ff12
@andriihomiak
test: Increase retry count and enable fail-fast
710e656
@andriihomiak
test: Increase retry count and enable fail-fast
4668b25
@andriihomiak
test: Decrease retry and remove time limit on retries
5ad162e
@andriihomiak
test: Add wait between failed tests
988732c
@andriihomiak
test: Mark flaky tests with xfail
91897b5
@andriihomiak
ci: Remove fail-fast and parallel job limit
9238515
@andriihomiak
ci: Run only 1 ci job at once
992638d
@andriihomiak
Copy link
Contributor Author

Test finally passed, but from now on when running e2e tests we need to check test logs, as some tests might xfail - if there are only a few, that do not reproduce consistently over the same os, chances are it is due to platform instability.

Resolving #512 might help alleviate this problem a little.

@andriihomiak andriihomiak linked an issue May 27, 2022 that may be closed by this pull request
Refactor method main._data_cp to simplify it #134
Closed
@andriihomiak andriihomiak merged commit 47d3544 into master May 27, 2022
@andriihomiak andriihomiak deleted the refactor/data_cp branch May 27, 2022 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YevheniiSemendiak YevheniiSemendiak YevheniiSemendiak approved these changes

Assignees

@andriihomiak andriihomiak

Labels
data_ingestion Issue on `neuro-extras data cp` functionality python Pull requests that update Python code
Projects
None yet
Milestone
No milestone
2 participants
@andriihomiak @YevheniiSemendiak