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.

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

Remove symlink of backend data files #171

Merged
merged 5 commits into from
Dec 4, 2019

Conversation

rlizzo
Copy link
Member

@rlizzo rlizzo commented Nov 27, 2019

Motivation and Context

Why is this change required? What problem does it solve?:

Remove symlink of data files between stage, store, and remote holding directories. This is both a confusing practice, and incompatible with privileges on some platforms (google colab connected to a mounted drive).

If it fixes an open issue, please link to the issue here:

Description

Describe your changes in detail:

replace symlink data files with touched files in same staging, remote and store locations.

At time of writing, am not 100% certain this won't break backwards compatibility with 0.4 symlinks. It shouldn't (since general structure and naming conventions are honored) but need tests to prove it.

Types of changes

What types of changes does your code introduce? Put an x in all the boxes that apply:

  • Documentation update
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Is this PR ready for review, or a work in progress?

  • Ready for review
  • Work in progress

How Has This Been Tested?

Put an x in the boxes that apply:

  • Current tests cover modifications made
  • New tests have been added to the test suite
  • Modifications were made to existing tests to support these changes
  • Tests may be needed, but they are not included when the PR was proposed
  • I don't know. Help!

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have signed (or will sign when prompted) the tensorwork CLA.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@rlizzo rlizzo added the enhancement New feature or request label Nov 27, 2019
@rlizzo rlizzo added this to the v0.5.0 milestone Nov 27, 2019
@rlizzo rlizzo self-assigned this Nov 27, 2019
@rlizzo rlizzo added the WIP Don't merge; Work in Progress label Nov 27, 2019
@codecov
Copy link

codecov bot commented Nov 27, 2019

Codecov Report

Merging #171 into master will decrease coverage by 0.05%.
The diff coverage is 94.48%.

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
- Coverage   95.28%   95.23%   -0.05%     
==========================================
  Files          66       66              
  Lines       11878    11862      -16     
  Branches     1029     1011      -18     
==========================================
- Hits        11317    11296      -21     
+ Misses        374      370       -4     
- Partials      187      196       +9
Impacted Files Coverage Δ
src/hangar/dataloaders/tfloader.py 100% <100%> (ø) ⬆️
src/hangar/dataloaders/torchloader.py 100% <100%> (ø) ⬆️
src/hangar/__init__.py 100% <100%> (ø) ⬆️
tests/test_dataloaders.py 100% <100%> (+1.12%) ⬆️
tests/test_cli.py 99.59% <100%> (+0.01%) ⬆️
src/hangar/utils.py 95.49% <100%> (+2.41%) ⬆️
src/hangar/backends/hdf5_01.py 88.66% <81.25%> (-1.49%) ⬇️
src/hangar/backends/hdf5_00.py 91.67% <84.38%> (-1.32%) ⬇️
src/hangar/backends/numpy_10.py 90.3% <86.67%> (-2.61%) ⬇️
src/hangar/records/commiting.py 91.06% <86.67%> (+1.14%) ⬆️
... and 5 more

@tensorwerk tensorwerk deleted a comment from lgtm-com bot Dec 4, 2019
@tensorwerk tensorwerk deleted a comment from lgtm-com bot Dec 4, 2019
@tensorwerk tensorwerk deleted a comment from lgtm-com bot Dec 4, 2019
@tensorwerk tensorwerk deleted a comment from lgtm-com bot Dec 4, 2019
@rlizzo rlizzo changed the title WIP: Remove symlink of backend data files Remove symlink of backend data files Dec 4, 2019
@rlizzo rlizzo added Awaiting Review Author has determined PR changes area nearly complete and ready for formal review. and removed WIP Don't merge; Work in Progress labels Dec 4, 2019
@rlizzo
Copy link
Member Author

rlizzo commented Dec 4, 2019

@hhsecond please review.

@tensorwerk tensorwerk deleted a comment from lgtm-com bot Dec 4, 2019
Copy link
Member

@hhsecond hhsecond left a comment

Choose a reason for hiding this comment

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

Other than minor changes with Path, looks good

src/hangar/backends/hdf5_00.py Outdated Show resolved Hide resolved
src/hangar/backends/hdf5_00.py Outdated Show resolved Hide resolved
@rlizzo rlizzo merged commit 90db078 into tensorwerk:master Dec 4, 2019
@rlizzo rlizzo added Resolved and removed Awaiting Review Author has determined PR changes area nearly complete and ready for formal review. labels Dec 5, 2019
@rlizzo rlizzo deleted the remove-symlinks branch December 5, 2019 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Symbolic Link Priveliges
2 participants