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

Sweep: Fix "IO error: file already open in python.exe" error when running dbt build after updating defer config #1185

Closed

Conversation

sweep-ai[bot]
Copy link
Contributor

@sweep-ai sweep-ai bot commented May 31, 2024

Purpose

This pull request addresses an issue where users are encountering an "IO error: file already open in python.exe" error when running the dbt build command after updating the defer config.

Description

The changes in this pull request aim to resolve the issue by ensuring that any Python threads created by the dbt project are properly closed after the defer config is applied. Specifically, the changes include:

  1. In the dbtCloudIntegration.ts and dbtCoreIntegration.ts files, the Python bridge used by various dbt commands (e.g., compileQuery, validateSql, getCatalog) is now closed in a finally block to ensure it is always cleaned up, even in the event of an exception.
  2. In the insightsPanel.ts file, the applyDeferConfig method now explicitly closes the Python bridge for the dbt project after the defer config is applied.

These changes should prevent the "IO error: file already open in python.exe" issue from occurring when running dbt build after updating the defer config.

Summary

  • Updated dbtCloudIntegration.ts and dbtCoreIntegration.ts to close the Python bridge used by various dbt commands in a finally block
  • Updated insightsPanel.ts to explicitly close the Python bridge for the dbt project after applying the defer config

Fixes #1184.


Tip

To get Sweep to edit this pull request, you can:

  • Comment below, and Sweep can edit the entire PR
  • Comment on a file, Sweep will only modify the commented file
  • Edit the original issue to get Sweep to recreate the PR from scratch

This is an automated message generated by Sweep AI.

Copy link
Contributor Author

sweep-ai bot commented May 31, 2024

Rollback Files For Sweep

  • Rollback changes to src/dbt_client/dbtCloudIntegration.ts
  • Rollback changes to src/dbt_client/dbtCoreIntegration.ts
  • Rollback changes to src/webview_provider/insightsPanel.ts

This is an automated message generated by Sweep AI.

@saravmajestic saravmajestic changed the base branch from main to master May 31, 2024 18:40
Copy link
Contributor Author

sweep-ai bot commented May 31, 2024

Sweep: PR Review

Authors: sweep-ai[bot], saravmajestic

This pull request ensures that the manifest merging process in the safe_parse_project function is executed within the context of a named connection when defer_to_prod is enabled and the dbt version is less than 1.8.

The main change was to introduce a connection context by using self.adapter.connection_named("master") before opening the manifest file. This was done to ensure that the adapter's connection is properly set during the file opening and merging process.

The with open(self.defer_to_prod_manifest_path) as f: block was nested within the with self.adapter.connection_named("master"): block. This adjustment ensures that the manifest merging process is executed within the context of a named connection, which may be necessary for certain adapter operations.


Sweep Found These Issues

  • In dbt_core_integration.py: The introduction of the connection context using self.adapter.connection_named("master") may cause issues if the connection named "master" is not properly configured or available, leading to potential runtime errors.
  • with self.adapter.connection_named("master"):
    with open(self.defer_to_prod_manifest_path) as f:
    manifest = WritableManifest.from_dict(json.load(f))
    selected = set()
    self.dbt.merge_from_artifact(
    self.adapter,
    other=manifest,
    selected=selected,
    favor_state=self.favor_state,
    )
    except Exception as e:

    View Diff


@mdesmet mdesmet self-requested a review June 2, 2024 14:09
@mdesmet
Copy link
Contributor

mdesmet commented Jun 2, 2024

One issue that I see with this approach is that it will wake up the database connection on load. Parsing shouldn't be needing a database connection.

Can we check that this connects/wakes up the warehouse in case of snowflake or bigquery?

@saravmajestic
Copy link
Collaborator

This change is not needed anymore, as it works with dbt 1.8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting error "IO error: file already open in python.exe" while running dbt build after applying defer config
2 participants