Copy dir if symlink fails #7447

anjutiwari · 2023-04-24T15:47:02Z

resolves #7428
resolves #8223

Description

dbt deps command fails to run in databricks DBFS. Copy dir in case if any symlink fails with expection

Checklist

I have read the contributing guide and understand what's expected of me
I have signed the CLA
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
I have opened an issue to add/update docs, or docs changes are not required/relevant for this PR
I have run changie new to create a changelog entry

nitinbhojwani · 2023-04-24T17:57:10Z

core/dbt/deps/local.py

        if system.path_exists(dest_path):
            if not system.path_is_symlink(dest_path):
                system.rmdir(dest_path)
            else:
                system.remove_file(dest_path)
-
-        if can_create_symlink:


if can_create_symlink check shouldn't be removed. It helps for the cases where symlink function doesn't exist on os, in which case AttributeError would be thrown.

except OSError should be added as an edge case like ours, where os.symlink function exists but it still fails.

Something like:

symlink_supported = False if can_create_symlink: try: fire_event(DepsCreatingLocalSymlink()) system.make_symlink(src_path, dest_path) except OSError: pass else: symlink_supported = True if not symlink_supported: fire_event(DepsSymlinkNotAvailable()) shutil.copytree(src_path, dest_path)

I do not think we need it, make_symlink calling it. The functionality remains the same. Check make_symlink method.
https://github.com/anjutiwari/dbt-core/blob/main/core/dbt/clients/system.py#L156
https://docs.getdbt.com/faqs/core/install-pip-os-prereqs.md

make_symlink method will throw dbt.exceptions.SymbolicLinkError, in which case except OSError block here won't handle it.
The behaviour is changed for the case where symlink function attribute isn't present on os

yes, I think we should use dbt.exceptions.SymbolicLinkError, and we should also catch the SymbolicLinkError.

Also, I am curious about which OS does not have symlinks function in python lib(os) package, as mentioned in my Issue description.
As per the dbt, we have a list of supported operating systems and condition on python3.7+. https://docs.getdbt.com/faqs/core/install-pip-os-prereqs.md

If my understanding is true, we may not have any scenarios where lib(os) package will not have a symlink function and we may not need to handle the SymbolicLinkError exception.

Waiting for the core reviewers to help us here.

nitinbhojwani · 2023-04-24T18:00:03Z

test/unit/test_deps.py

+        LocalPinnedPackage("local").install("dummy", "dummy")
+        self.assertEqual(mock_shutil.call_count, 1)
+        mock_shutil.assert_called_once_with("/tmp/source", "/tmp/dest")
+


add test where system.supports_symlinks() returns False

My idea was to handle the tests for only the newly added code.

nitinbhojwani

system.supports_symlinks() check should continue to exist before trying to invoke symlink on os

Along with that, the edge case handling of OSError exception on system.make_symlink should be done.

anjutiwari · 2023-04-25T01:06:52Z

core/dbt/deps/local.py

            fire_event(DepsCreatingLocalSymlink())
            system.make_symlink(src_path, dest_path)
-
-        else:
+        except OSError:


https://docs.getdbt.com/faqs/core/install-pip-os-prereqs.md
Handling exception case of DBFS

chronitis · 2023-05-03T06:48:17Z

Just a note that I think this PR fixes wider problems than just those related to databricks DBFS.

When using a local: PATH package on Windows, DBT will attempt to create a symlink (since supports_symlink() is true), but unless the user has administrator permissions or specific group policy, creating the symlink fails. If I read this PR right, it would also fix this issue.

anjutiwari · 2023-05-04T06:16:43Z

Thanks @chronitis for pointing out another edge case. Let me try to reproduce this scenario and verify if this solution is already fixing the problem.

anjutiwari · 2023-05-30T12:03:07Z

Hey @jtcohen6,
Just wanted to check for any update on this. Please let me know if I could make this PR more smooth to get it prioritized for the review.

Thank you

gshank · 2023-06-28T14:33:37Z

Sorry for the delay in processing this. We are getting close to a release and it would be nice to get this in. Could you fix the merge conflict in test_deps.py?

anjutiwari · 2023-06-30T17:25:45Z

Thanks @gshank. Fixing it.

codecov · 2023-07-20T16:52:03Z

Codecov Report

Merging #7447 (abb6c24) into main (44d1e73) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #7447      +/-   ##
==========================================
- Coverage   86.28%   86.26%   -0.02%     
==========================================
  Files         174      174              
  Lines       25518    25518              
==========================================
- Hits        22019    22014       -5     
- Misses       3499     3504       +5

Files Changed	Coverage Δ
core/dbt/deps/local.py	`96.00% <100.00%> (+4.00%)`	⬆️

... and 1 file with indirect coverage changes

dbeatty10 · 2023-08-08T12:55:44Z

Just a note that I think this PR fixes wider problems than just those related to databricks DBFS.

When using a local: PATH package on Windows, DBT will attempt to create a symlink (since supports_symlink() is true), but unless the user has administrator permissions or specific group policy, creating the symlink fails. If I read this PR right, it would also fix this issue.

Very good point @chronitis ! This PR does look like it addresses use cases beyond Databricks DBFS, namely any time a symlink doesn't work. For example, it would also resolve #8223.

.changes/unreleased/Fixes-20230424-210734.yaml

Co-authored-by: Doug Beatty <[email protected]>

…o anjutiwari-CT-2457/dbt-deps

Co-authored-by: Anju <[email protected]>

anjutiwari added 2 commits April 24, 2023 19:51

Copy dir if symlink fails

28875a6

Copy dir if symlink fails

ffa6d01

anjutiwari requested review from a team as code owners April 24, 2023 15:47

anjutiwari requested review from emmyoop and gshank April 24, 2023 15:47

cla-bot bot added the cla:yes label Apr 24, 2023

nitinbhojwani reviewed Apr 24, 2023

View reviewed changes

nitinbhojwani suggested changes Apr 24, 2023

View reviewed changes

anjutiwari commented Apr 25, 2023

View reviewed changes

jtcohen6 added Team:Language ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering labels Apr 25, 2023

Merge branch 'main' into CT-2457/dbt-deps

56087c4

jtcohen6 removed the Team:Language label Jul 19, 2023

gshank closed this Jul 20, 2023

gshank reopened this Jul 20, 2023

VersusFacit added the test postgres Run integration tests for the Postgres adapter in CI. label Aug 8, 2023

dbeatty10 mentioned this pull request Aug 8, 2023

[CT-2876] [Feature] add possibility to copy dbt local packages instead of make it symlink #8223

Open

3 tasks

dbeatty10 reviewed Aug 8, 2023

View reviewed changes

.changes/unreleased/Fixes-20230424-210734.yaml Outdated Show resolved Hide resolved

gshank closed this Aug 8, 2023

gshank reopened this Aug 8, 2023

Update .changes/unreleased/Fixes-20230424-210734.yaml

a4d28cf

Co-authored-by: Doug Beatty <[email protected]>

gshank added 2 commits August 8, 2023 19:39

Merge branch 'CT-2457/dbt-deps' of github.com:anjutiwari/dbt-core int…

1bbd247

…o anjutiwari-CT-2457/dbt-deps

fix formatting of changelog

abb6c24

gshank approved these changes Aug 9, 2023

View reviewed changes

gshank merged commit 8c98ef3 into dbt-labs:main Aug 9, 2023
47 of 48 checks passed

aranke added the backport 1.6.latest label Aug 31, 2023

aranke pushed a commit that referenced this pull request Sep 5, 2023

Copy dir if symlink fails (#7447)

deaf9aa

aranke added a commit that referenced this pull request Sep 5, 2023

Copy dir if symlink fails (#7447) (#8548)

78bb854

Co-authored-by: Anju <[email protected]>

tlento mentioned this pull request Nov 6, 2023

Update typing-extensions version to >=4.4 #9012

Merged

5 tasks

tlento mentioned this pull request Feb 26, 2024

Update dbt-semantic-interfaces dependency to compatible range #9671

Merged

aranke mentioned this pull request Jul 12, 2024

test env vars casing 1 6 latest #10437

Draft

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy dir if symlink fails #7447

Copy dir if symlink fails #7447

anjutiwari commented Apr 24, 2023 •

edited by dbeatty10

Loading

nitinbhojwani Apr 24, 2023

anjutiwari Apr 25, 2023 •

edited

Loading

nitinbhojwani Apr 25, 2023

anjutiwari Apr 25, 2023

nitinbhojwani Apr 24, 2023

anjutiwari Apr 25, 2023 •

edited

Loading

nitinbhojwani left a comment

anjutiwari Apr 25, 2023

chronitis commented May 3, 2023

anjutiwari commented May 4, 2023

anjutiwari commented May 30, 2023

gshank commented Jun 28, 2023

anjutiwari commented Jun 30, 2023

codecov bot commented Jul 20, 2023 •

edited

Loading

dbeatty10 commented Aug 8, 2023

Copy dir if symlink fails #7447

Copy dir if symlink fails #7447

Conversation

anjutiwari commented Apr 24, 2023 • edited by dbeatty10 Loading

Description

Checklist

nitinbhojwani Apr 24, 2023

Choose a reason for hiding this comment

anjutiwari Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

nitinbhojwani Apr 25, 2023

Choose a reason for hiding this comment

anjutiwari Apr 25, 2023

Choose a reason for hiding this comment

nitinbhojwani Apr 24, 2023

Choose a reason for hiding this comment

anjutiwari Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

nitinbhojwani left a comment

Choose a reason for hiding this comment

anjutiwari Apr 25, 2023

Choose a reason for hiding this comment

chronitis commented May 3, 2023

anjutiwari commented May 4, 2023

anjutiwari commented May 30, 2023

gshank commented Jun 28, 2023

anjutiwari commented Jun 30, 2023

codecov bot commented Jul 20, 2023 • edited Loading

Codecov Report

dbeatty10 commented Aug 8, 2023

anjutiwari commented Apr 24, 2023 •

edited by dbeatty10

Loading

anjutiwari Apr 25, 2023 •

edited

Loading

anjutiwari Apr 25, 2023 •

edited

Loading

codecov bot commented Jul 20, 2023 •

edited

Loading