Fix/changes current working dir when using a dbt project dir #9596

rariyama · 2024-02-19T05:17:04Z

resolves #8997
I closed the previous PR by mistake, then I submit this one.

Problem

There is a difference in behaviour between dbt cli and dbt.invoke.
For example, When executing dbtRunner().invoke(["deps", "--project-dir", "project_dir"]) in python, directory
changes to project_dir, even though it doesn't affect when executing dbt deps.

Solution

The reason why directory changes is dbt.task.base.move_to_nearest_project_dir() is called in CleanTask , DepsTask and InitTask . Directory changes to the argument of the function.
It'll be possible to resolve this problem by using those classes as a context manager. The change will happen only in with statement.

Checklist

I have read the contributing guide and understand what's expected of me
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
This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX
This PR includes type annotations for new and modified functions

ChenyuLInx

Thanks for adding this @rariyama
I wonder what if you can add the __enter__ and __exit__ to just the base task so we only do it once?

Also tagging @MichelleArk to see if there are any thoughts on starting this with pattern. It looks good to me.

rariyama · 2024-02-21T08:49:21Z

Hi @ChenyuLInx . Thanks for your comments.
I agree with your idea, then I updated my code as follows on this commit 31ba979.

Added __enter__ and __exit__ to BaseTask
each child TaskClass inherits that class and behaves as a context manager.

I would like to add test code for this update. I feel like it's good enough to add the following code to integration test but what do you think about?

    def test_dbtRunner_invoke(self):
        pd = "<test-project-path>"
        dr = dbtRunner()
        commands = ["deps", "clean", "init"]

        for command in commands:
            req = [command, "--project-dir", pd, "--profiles-dir", pd]
            before_dir = os.getcwd()
            res = dr.invoke(req)
            self.assertEqual(res.success, True)
            # MEMO: Test whether directory is not changed after command is invoked.

            self.assertEqual(before_dir, os.getcwd())

dbeatty10 · 2024-08-15T04:45:28Z

@rariyama I saw your message in #8997 (comment) and checked the comments in this PR.

Could you do these two things? After that we can have one of our engineers take another look:

Resolve the conflicts
Add the test below

    def test_runner_invoke(self, project):
        project_dir = project.project_root
        runner = dbtRunner()
        commands = ["deps", "clean", "init"]
        
        # TODO: change to some other directory other than the dbt project root

        for command in commands:
            before_dir = os.getcwd()
            res = runner.invoke([command, "--project-dir", project_dir, "--profiles-dir", project_dir])
            self.assertEqual(res.success, True)
            self.assertEqual(before_dir, os.getcwd())

The key thing will be to make sure the test actually captures the scenario in the bug report. i.e., you'll want to make sure that the test fails without your changes, but the test passes after you changes.

rariyama · 2024-08-15T18:02:34Z

Hi @dbeatty10. Thank you for checking my comment.
I resolved conflict in this commit fc22afd and add test assertion in the commit c75835b.

I confirmed the results of both unittest and integration test are as expected by following ways.

Added test assertion without my changes and integration test fails.
- Added test assertion to the source code of main branch and test failed.
Added my changes and both unit test and integration test passes.
- It means my changes can resolve the issue.

I run the test code by the command make test and make integration.

If there's no remaining tasks I have to do, could you please have the engineers review it?

codecov · 2024-08-15T19:38:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.92%. Comparing base (9ca1bc5) to head (b7aa1c6).
Report is 16 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9596      +/-   ##
==========================================
- Coverage   88.93%   88.92%   -0.02%     
==========================================
  Files         180      180              
  Lines       22735    22760      +25     
==========================================
+ Hits        20220    20239      +19     
- Misses       2515     2521       +6

Flag	Coverage Δ
integration	`86.23% <100.00%> (-0.02%)`	⬇️
unit	`62.33% <11.76%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
Unit Tests	`62.33% <11.76%> (-0.05%)`	⬇️
Integration Tests	`86.23% <100.00%> (-0.02%)`	⬇️

rariyama · 2024-08-20T02:36:38Z

Hello Team. Could someone please review my PR?

dbeatty10 · 2024-08-20T21:09:13Z

core/dbt/tests/util.py

+
+    before_dir = os.getcwd()
    res = dbt.invoke(args)
+    after_dir = os.getcwd()
+    # The directory has not been changed after running dbt command.
+    assert before_dir == after_dir


It seems like this assertion would be better if it were directly within a test rather than here. Maybe here instead?

What do you think @ChenyuLInx?

I think this is great!! Also makes it checking all future commands.

I added test code to tests/functional/dbt_runner/test_dbt_runner.py in this commit.
26ec953

dbeatty10 · 2024-08-20T21:40:04Z

core/dbt/task/deps.py

+        project.project_root = str(Path(project.project_root).resolve())
        super().__init__(args=args)
        # N.B. This is a temporary fix for a bug when using relative paths via
        # --project-dir with deps.  A larger overhaul of our path handling methods
        # is needed to fix this the "right" way.
        # See GH-7615
-        project.project_root = str(Path(project.project_root).resolve())
-        self.project = project
-
-        move_to_nearest_project_dir(project.project_root)
        self.cli_vars = args.vars
+        self.project = project


@rariyama Did you happen to check if preserving this order worked or not?

I'm wondering if you changed the ordering intentionally or if it was just an accidental side-effect when resolving merge conflicts.

i.e., I'm curious if this would work (which is basically identical to here except for removing the move_to_nearest_project_dir(project.project_root) line):

super().__init__(args=args) # N.B. This is a temporary fix for a bug when using relative paths via # --project-dir with deps. A larger overhaul of our path handling methods # is needed to fix this the "right" way. # See GH-7615 project.project_root = str(Path(project.project_root).resolve()) self.project = project self.cli_vars = args.vars

I fixed the order of variables as you wrote in this commit.
4191735
I confirmed integration test passed on my local environment.

ChenyuLInx

Thanks @rariyama for thinking through this and submit this PR. This change makes sense to me!
Since this is something that modifies the behavior of directory I would like to play with it a bit more locally before giving it the approve and merge.

…t_dir1' of https://github.com/rariyama/dbt-core into fix/changes_current_working_dir_when_using_a_dbt_project_dir1

rariyama · 2024-08-21T08:12:36Z

@ChenyuLInx
Thank you for checking my PR. Please try to run on your local environment. If there are something you want to ask or I need to fix, please tell me about that.

dbeatty10 · 2024-08-21T13:24:06Z

core/dbt/task/deps.py

@@ -202,6 +200,7 @@ def lock(self) -> None:
        fire_event(DepsLockUpdating(lock_filepath=lock_filepath))

    def run(self) -> None:
+        move_to_nearest_project_dir(self.args.project_dir)


Did move_to_nearest_project_dir(self.args.project_dir) need to move to this location in order for things to work?

Or was it "nice-to-have" for some reason?

There is a reason for moving the move_to_nearest_project_dir() function. The DepsTask class inherits from the BaseTask class and is designed to behave as a context manager. In BaseTask, the current directory is obtained within __enter__(), and os.chdir() is called within __exit__() to return to the previously obtained directory.

Normally, since __init__() is executed before __enter__(), if move_to_nearest_project_dir() is called inside __init__(), the directory after the move will be recorded, and it will not return correctly to the original directory.

The sequence of events can be described as follows:

__init__().move_to_nearest_project_dir() # Moves to the project directory.

__enter__().os.getcwd() # Gets the current directory, which is the project directory.

__exit__().os.chdir() # Moves back to the project directory obtained in step 2.

By moving the move_to_nearest_project_dir() function to the run() function, the order of execution changes as follows:

__enter__().os.getcwd() # Gets the current directory, i.e., the directory where dbt run is executed.

DepsTask.run().move_to_nearest_project_dir() # Moves to the project directory to execute dbt deps.

__exit__().os.chdir() # Returns to the directory obtained in step 1.

This is why it was necessary to move the move_to_nearest_project_dir() function.

That is an amazing explanation -- thank you!

ChenyuLInx · 2024-08-21T22:14:50Z

@rariyama looks like the new test you just introduced is failing on the deps command.(I think this is due to missing project fixture as input arg to the test.
def test_directory_does_not_change(self, dbt: dbtRunner, project) -> None:
I pushed a commit to your branch as this is a small fix, hope that's okay with you.
I did some local testing with dbtRunner and the behavior looks good to me.

ChenyuLInx · 2024-09-03T20:42:53Z

@rariyama I went ahead and merged this change! Thank you so much for fixing it!

rariyama · 2024-09-04T01:57:15Z

Hi @ChenyuLInx
Thank you for the review. If I have another request for a review in the future, I would appreciate your assistance again.

rariyama added 2 commits December 6, 2023 19:59

made class changing directory a context manager.

22c4f0a

add change log

08093c9

rariyama requested a review from a team as a code owner February 19, 2024 05:17

rariyama requested a review from ChenyuLInx February 19, 2024 05:17

cla-bot bot added the cla:yes label Feb 19, 2024

rariyama added 2 commits February 19, 2024 14:27

fix conflict

1ba7e3c

fix conflict

398cd0b

ChenyuLInx reviewed Feb 20, 2024

View reviewed changes

ChenyuLInx requested a review from MichelleArk February 20, 2024 22:31

made base as a context manager

31ba979

dbeatty10 added the community This PR is from a community member label Mar 22, 2024

rariyama added 2 commits August 15, 2024 23:37

resolve conflict

fc22afd

add assertion

c75835b

Remove index.html

17c79da

dbeatty10 reviewed Aug 20, 2024

View reviewed changes

ChenyuLInx reviewed Aug 21, 2024

View reviewed changes

rariyama added 3 commits August 21, 2024 15:08

add it test to testDbtRunner

26ec953

fix deps args order

4191735

Merge branch 'fix/changes_current_working_dir_when_using_a_dbt_projec…

c0e9788

…t_dir1' of https://github.com/rariyama/dbt-core into fix/changes_current_working_dir_when_using_a_dbt_project_dir1

dbeatty10 reviewed Aug 21, 2024

View reviewed changes

fix test

b7aa1c6

ChenyuLInx approved these changes Sep 3, 2024

View reviewed changes

ChenyuLInx merged commit b56d96d into dbt-labs:main Sep 3, 2024
59 of 60 checks passed

ofek1weiss mentioned this pull request Sep 29, 2024

Upgrading version 0.15.1 → 0.16.0 cause "Runtime Error: Could not find profile named 'elementary" elementary-data/elementary#1702

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/changes current working dir when using a dbt project dir #9596

Fix/changes current working dir when using a dbt project dir #9596

rariyama commented Feb 19, 2024

ChenyuLInx left a comment

rariyama commented Feb 21, 2024

dbeatty10 commented Aug 15, 2024

rariyama commented Aug 15, 2024

codecov bot commented Aug 15, 2024 •

edited

Loading

rariyama commented Aug 20, 2024

dbeatty10 Aug 20, 2024

ChenyuLInx Aug 21, 2024

rariyama Aug 21, 2024

dbeatty10 Aug 20, 2024

rariyama Aug 21, 2024 •

edited

Loading

ChenyuLInx left a comment

rariyama commented Aug 21, 2024

dbeatty10 Aug 21, 2024

rariyama Aug 21, 2024

dbeatty10 Aug 21, 2024

ChenyuLInx commented Aug 21, 2024

ChenyuLInx commented Sep 3, 2024

rariyama commented Sep 4, 2024

Fix/changes current working dir when using a dbt project dir #9596

Fix/changes current working dir when using a dbt project dir #9596

Conversation

rariyama commented Feb 19, 2024

Problem

Solution

Checklist

ChenyuLInx left a comment

Choose a reason for hiding this comment

rariyama commented Feb 21, 2024

dbeatty10 commented Aug 15, 2024

rariyama commented Aug 15, 2024

codecov bot commented Aug 15, 2024 • edited Loading

Codecov Report

rariyama commented Aug 20, 2024

dbeatty10 Aug 20, 2024

Choose a reason for hiding this comment

ChenyuLInx Aug 21, 2024

Choose a reason for hiding this comment

rariyama Aug 21, 2024

Choose a reason for hiding this comment

dbeatty10 Aug 20, 2024

Choose a reason for hiding this comment

rariyama Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

ChenyuLInx left a comment

Choose a reason for hiding this comment

rariyama commented Aug 21, 2024

dbeatty10 Aug 21, 2024

Choose a reason for hiding this comment

rariyama Aug 21, 2024

Choose a reason for hiding this comment

dbeatty10 Aug 21, 2024

Choose a reason for hiding this comment

ChenyuLInx commented Aug 21, 2024

ChenyuLInx commented Sep 3, 2024

rariyama commented Sep 4, 2024

codecov bot commented Aug 15, 2024 •

edited

Loading

rariyama Aug 21, 2024 •

edited

Loading