Skip to content

Fix missing dependencies#1485

Merged
jimidle merged 11 commits intomainfrom
fix-missing-dependencies
Mar 27, 2025
Merged

Fix missing dependencies#1485
jimidle merged 11 commits intomainfrom
fix-missing-dependencies

Conversation

@ericvergnaud
Copy link
Contributor

@ericvergnaud ericvergnaud commented Mar 10, 2025 •
edited
Loading

This PR:

  • adds standard-distutils conditional dependency. Starting with Python 3.11, distutils is no longer part of python, so needs to be installed.

@ericvergnaud ericvergnaud requested a review from a team as a code owner March 10, 2025 15:30
@github-actions
Copy link

github-actions bot commented Mar 10, 2025 •
edited
Loading

✅ 13/13 passed, 1 skipped, 9s total

Running from acceptance #342

@ericvergnaud ericvergnaud mentioned this pull request Mar 11, 2025
Closed
sundarshankar89
sundarshankar89 requested changes Mar 17, 2025
View reviewed changes
pyproject.toml Outdated
]
dependencies = [
"databricks-sdk>=0.29,<0.42",
"databricks-connect==15.1",
Copy link
Collaborator

@sundarshankar89 sundarshankar89 Mar 17, 2025

Choose a reason for hiding this comment

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

Need to understand why this change is needed, databricks-connect is dependant on cluster_id or warehouse_id, so we can't fix it to specific version, it is driven by labs.yml configuration hence it is a test only depadancy.

Also why is the standard-distuilts is tagged to python version above the base line 3.10 we support?

Copy link
Contributor Author

@ericvergnaud ericvergnaud Mar 17, 2025

Choose a reason for hiding this comment

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

distutils is part of python 3.10, but removed starting with 3.11. If running python > 3.10 it needs to be added.

Copy link
Contributor Author

@ericvergnaud ericvergnaud Mar 17, 2025

Choose a reason for hiding this comment

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

databricks-connect is required when running the cli locally. Happy to remove it if it's in the way, but please provide a means to run the cli locally

Copy link
Collaborator

@sundarshankar89 sundarshankar89 Mar 17, 2025

Choose a reason for hiding this comment

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

labs.yml should have this, but I want to understand why is databricks-connect needed for transpile.

install:
  min_runtime_version: 13.3
  require_running_cluster: false
  require_databricks_connect: true

Copy link
Contributor Author

@ericvergnaud ericvergnaud Mar 17, 2025

Choose a reason for hiding this comment

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

IIRC it's needed during install-transpile , because it loads all configs and recon related code

Copy link
Collaborator

@sundarshankar89 sundarshankar89 Mar 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thats why moving it as part of the configure instead of run will avoid this.

def configure(self, module: str) -> RemorphConfigs:

Copy link
Collaborator

@sundarshankar89 sundarshankar89 Mar 18, 2025

Choose a reason for hiding this comment

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

@ericvergnaud I was able to reproduce and understood the bug better, these changes should fix this.
#1495

Copy link
Contributor Author

@ericvergnaud ericvergnaud Mar 18, 2025

Choose a reason for hiding this comment

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

Noted. Updated this PR accordingly.

Copy link
Contributor Author

@ericvergnaud ericvergnaud Mar 20, 2025

Choose a reason for hiding this comment

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

@sundarshankar89 ready to merge

sundarshankar89 and others added 7 commits March 18, 2025 15:34
@sundarshankar89
Seperated input and out configs for recon to ease the necessity for d…
79ae631 
…bconnect
@sundarshankar89
fixed typo
adb1302
@sundarshankar89
Merge branch 'main' into patch/reorganise_recon_config
0a01989
@sundarshankar89
moved constants around
16240ad
@sundarshankar89
Merge remote-tracking branch 'origin/patch/reorganise_recon_config' i…
72751b3 
…nto patch/reorganise_recon_config
@ericvergnaud
Merge branch 'patch/reorganise_recon_config' into fix-missing-depende…
b86073e 
…ncies

* patch/reorganise_recon_config:
  moved constants around
  fixed typo
  Update transpilers.md (#1486)
  Seperated input and out configs for recon to ease the necessity for dbconnect
  Drop proxy commands (#1493)
  Fix labs.yml (#1490)
  Configure lsp custom options (#1487)
  Add support for DBT (#1456)
@ericvergnaud
undo change
61fba5e
@ericvergnaud ericvergnaud added the stacked PR Should be reviewed, but not merged label Mar 18, 2025
@ericvergnaud ericvergnaud requested review from a team and sundarshankar89 March 18, 2025 11:17
@ericvergnaud ericvergnaud removed the stacked PR Should be reviewed, but not merged label Mar 20, 2025
sundarshankar89
sundarshankar89 approved these changes Mar 27, 2025
View reviewed changes
Copy link
Collaborator

@sundarshankar89 sundarshankar89 left a comment

Choose a reason for hiding this comment

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

LGTM

@sundarshankar89 sundarshankar89 requested a review from a team March 27, 2025 15:33
jimidle
jimidle approved these changes Mar 27, 2025
View reviewed changes
Copy link
Contributor

@jimidle jimidle left a comment

Choose a reason for hiding this comment

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

LGTM

@jimidle jimidle added this pull request to the merge queue Mar 27, 2025
Merged via the queue into main with commit 5c8e46e Mar 27, 2025
6 checks passed
@jimidle jimidle deleted the fix-missing-dependencies branch March 27, 2025 15:43
ericvergnaud added a commit that referenced this pull request Apr 3, 2025
@ericvergnaud
Merge branch 'main' into enable-debug-logs-in-lsp-server
eaf82ca 
* main:
  Improve lsp resiliency (#1491)
  Hot Fix for Dashboard Names (#1476)
  Improve installer resiliency (#1484)
  Patch Contributing Document (#1506)
  Introduce Documentation for Remorph (#1460)
  Fix missing dependencies (#1485)
  Installation Improvements (#1495)
  Update transpilers.md (#1486)
  Drop proxy commands (#1493)
  Fix labs.yml (#1490)
  Configure lsp custom options (#1487)
  Add support for DBT (#1456)
  Introduce table definition (#1450)
  Add Error handling Pipeline Execution (#1466)
  Enhance Profiler config to execute Python Script (#1465)
  Pipeline Executor for Profiling (#1453)
  Updated document On Python requirement (#1473)

# Conflicts:
#	src/databricks/labs/remorph/transpiler/lsp/lsp_engine.py
ericvergnaud added a commit that referenced this pull request Apr 4, 2025
@ericvergnaud
Merge branch 'main' into make-transpile-config-flexible
ffd5c6d 
* main:
  Improve lsp resiliency (#1491)
  Hot Fix for Dashboard Names (#1476)
  Improve installer resiliency (#1484)
  Patch Contributing Document (#1506)
  Introduce Documentation for Remorph (#1460)
  Fix missing dependencies (#1485)
  Installation Improvements (#1495)
  Update transpilers.md (#1486)
  Drop proxy commands (#1493)
  Fix labs.yml (#1490)
  Configure lsp custom options (#1487)
  Add support for DBT (#1456)

# Conflicts:
#	pyproject.toml
#	src/databricks/labs/remorph/config.py
#	src/databricks/labs/remorph/install.py
#	src/databricks/labs/remorph/transpiler/execute.py
#	tests/unit/test_install.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jimidle jimidle jimidle approved these changes

@sundarshankar89 sundarshankar89 sundarshankar89 approved these changes

Assignees

@sundarshankar89 sundarshankar89

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ericvergnaud @jimidle @sundarshankar89