Fixing Dependencies in DbtRunner for Uninstalled Packages #1050
Conversation
elementary/clients/dbt/dbt_runner.py
Outdated
| @@ -241,3 +242,28 @@ def ls(self, select: Optional[str] = None) -> list: | |||
|
|
|||
| def source_freshness(self): | |||
| self._run_command(command_args=["source", "freshness"]) | |||
|
|
|||
| def get_installed_packages(self): | |||
| dbt_packages_folder = os.path.join(os.getcwd(), 'dbt_packages') | |||
There was a problem hiding this comment.
The dbt_packages directory will not be created relative to the working directory but rather to the dbt project directory. You can use the following in order to get its location.
elementary/clients/dbt/dbt_runner.py
Outdated
| return False | ||
|
|
||
| def _run_deps_if_needed(self): | ||
| packages_yaml_file = os.path.abspath('packages.yml') |
There was a problem hiding this comment.
Same as comment above, join packages.yml with the dbt project's path.
elementary/clients/dbt/dbt_runner.py
Outdated
| packages_yaml_file = os.path.abspath('packages.yml') | ||
| with open(packages_yaml_file, 'r') as file: | ||
| packages_data = yaml.safe_load(file) | ||
| package_names = [package_entry.get('package', '').split('/')[-1] for package_entry in packages_data['packages']] |
There was a problem hiding this comment.
Can you maybe add the if to the first list comprehension and remove the second?
| package_names = [package_entry.get('package', '').split('/')[-1] for package_entry in packages_data['packages']] | |
| package_names = [package_entry['package'].split('/')[-1] for package_entry in packages_data['packages'] if 'package' in package_entry] |
elementary/clients/dbt/dbt_runner.py
Outdated
| @@ -241,3 +242,28 @@ def ls(self, select: Optional[str] = None) -> list: | |||
|
|
|||
| def source_freshness(self): | |||
| self._run_command(command_args=["source", "freshness"]) | |||
|
|
|||
| def get_installed_packages(self): | |||
There was a problem hiding this comment.
Care to rename to get_installed_packages_names and make it a private method?
There was a problem hiding this comment.
Sure, I have made the necessary change in the latest commit
elementary/clients/dbt/dbt_runner.py
Outdated
| package_names = [package_entry.get('package', '').split('/')[-1] for package_entry in packages_data['packages']] | ||
| package_names = [package for package in package_names if package != ''] | ||
| installed_packages = self.get_installed_packages() | ||
| if not installed_packages: |
There was a problem hiding this comment.
What do you think about:
if set(required_package_names) != set(installed_package_names):
...There was a problem hiding this comment.
Sounds good, this looks much clean than the previous method.
|
Hi @elongl , I have made the changes you have mentioned in the review. Please do let me know if you further require any changes. Thanks! |
elementary/clients/dbt/dbt_runner.py
Outdated
|
|
||
| _PACKAGES_FILENAME = "packages.yml" | ||
|
|
||
| PATH = os.path.join(_MONITOR_DIR, "dbt_project") |
There was a problem hiding this comment.
Actually this does not make sense because it couples DbtRunner to the internal dbt project while it should remain generic. Instead, we should use self.project_dir and join with it everywhere.
Sorry for misleading you 🙏
There was a problem hiding this comment.
No worries, I have made the necessary changes in the latest commit. Just to be clear we will be doing a check on dbt_packages and packages.yml inside the self.project_dir to determine whether to use self.deps() or not, right?
Thank you for the opportunity! |
Description:
This PR addresses issue #1039 regarding DbtRunner's behavior when required packages are not installed in the target dbt project. Currently, DbtRunner is able to run against a project without its required packages installed, leading to potential issues during execution.