[PROD-37308] Preserve original databricks.sdk.runtime for internal purposes #96
Conversation
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #96 +/- ##
=======================================
Coverage 52.72% 52.72%
=======================================
Files 29 29
Lines 17067 17067
=======================================
Hits 8998 8998
Misses 8069 8069
☔ View full report in Codecov by Sentry. |
databricks/sdk/runtime/__init__.py
Outdated
| dbutils = RemoteDbUtils() | ||
| # OSS implementation | ||
| is_oss_implementation = True | ||
| dbutils = _get_global_dbutils() |
There was a problem hiding this comment.
Can you check if we can remove _get_global_dbutils function altogether? It’s meant to check if “dbutils” got injected into nonlocal namespace
There was a problem hiding this comment.
+1. here you are explicitly injecting dbutils into the globals namespace. So we don't really need to see if dbutils are injected when is_oss_implementation = True
There was a problem hiding this comment.
@nfx @kartikgupta-db Makes sense, may I know how to test whether it is OK to remove it though, is passing unit tests sufficient?
There was a problem hiding this comment.
We don’t have unit tests for this module yet.
| dbutils = _get_global_dbutils() | ||
|
|
||
| try: | ||
| from .stub import * |
There was a problem hiding this comment.
Stub is only there for VSCODE extension to show Autocompletion with documentation. It has no implementation, as you see.
@kartikgupta-db do you think we can remove stub and keep VSCode extension functioning properly?
There was a problem hiding this comment.
Not for now. We have started fully depending on the SDK for the stubs. So we need to keep them in.
There was a problem hiding this comment.
@kartikgupta-db can we somehow separate the stubs even further within the SDK?
databricks/sdk/runtime/__init__.py
Outdated
| # this assumes that all environment variables are set | ||
| dbutils = RemoteDbUtils() | ||
| except NameError: | ||
| from databricks.sdk.dbutils import RemoteDbUtils |
There was a problem hiding this comment.
Why do we need to import remote dbutils implementation twice?…
There was a problem hiding this comment.
@nfx This is the original OSS implementation, but the commit suggestion by @kartikgupta-db down seems to solve this issue.
databricks/sdk/runtime/__init__.py
Outdated
| dbutils = RemoteDbUtils() | ||
| # OSS implementation | ||
| is_oss_implementation = True | ||
| dbutils = _get_global_dbutils() |
There was a problem hiding this comment.
+1. here you are explicitly injecting dbutils into the globals namespace. So we don't really need to see if dbutils are injected when is_oss_implementation = True
Co-authored-by: Kartik Gupta <88345179+kartikgupta-db@users.noreply.github.com> Signed-off-by: Boyuan Deng <40481626+dby-tmwctw@users.noreply.github.com>
There was a problem hiding this comment.
Looks good to me, let’s wait for @kartikgupta-db to approve from VSCode part
Changes
We want to use
databricks-sdkin DBR but currently installing it breaks existingdatabricks.sdk.runtimemodule we added. This PR fix it by using the internal packages if existed, and fall-back to OSS implementation if that does not exist.Tests
Manual testing on E2 Dogfood
make testrun locallymake fmtappliedrelevant integration tests applied