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

Fixed bug for double-uploading of unreleased wheels in air-gapped setups #103

Merged
merged 1 commit into from
May 12, 2024

Conversation

nfx
Copy link
Collaborator

@nfx nfx commented May 12, 2024

With the changes from #99, we were not hitting if wheel.name == self._local_wheel.name condition for unreleased wheels, resulting in undefined behavior:

product_info = ProductInfo.from_class(WheelsV2)
with WheelsV2(new_installation, product_info) as whl:
    whl.upload_wheel_dependencies(["databricks"])
    installation_files = new_installation.files()
    # only Databricks SDK has to be uploaded
    assert len(installation_files) == 1

With the changes from #99, we were not hitting `if wheel.name == self._local_wheel.name` condition for unreleased wheels, resulting in undefined behavior:

```
product_info = ProductInfo.from_class(WheelsV2)
with WheelsV2(new_installation, product_info) as whl:
    whl.upload_wheel_dependencies(["databricks"])
    installation_files = new_installation.files()
    # only Databricks SDK has to be uploaded
    assert len(installation_files) == 1
```
@nfx nfx requested a review from aminmovahed-db May 12, 2024 10:31
Copy link

codecov bot commented May 12, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 78.78%. Comparing base (50b5474) to head (ab61bd4).

Files Patch % Lines
src/databricks/labs/blueprint/wheels.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #103      +/-   ##
==========================================
+ Coverage   78.72%   78.78%   +0.05%     
==========================================
  Files          14       14              
  Lines        1495     1499       +4     
  Branches      269      269              
==========================================
+ Hits         1177     1181       +4     
  Misses        230      230              
  Partials       88       88              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

✅ 12/12 passed, 2 skipped, 24s total

Running from acceptance #130

@nfx nfx merged commit 41e4aab into main May 12, 2024
13 checks passed
@nfx nfx deleted the fix/upstream-whls branch May 12, 2024 10:33
nfx added a commit that referenced this pull request May 12, 2024
* Added upstream wheel uploads for Databricks Workspaces without Public Internet access ([#99](#99)). This commit introduces a new feature for uploading upstream wheel dependencies to Databricks Workspaces without Public Internet access. A new flag has been added to upload functions, allowing users to include or exclude dependencies in the download list. The `WheelsV2` class has been updated with a new method, `upload_wheel_dependencies(prefixes)`, which checks if each wheel's name starts with any of the provided prefixes before uploading it to the Workspace File System (WSFS). This feature also includes two new tests to verify the functionality of uploading the main wheel package and dependent wheel packages, optimizing downloads based on specific use cases. This enables users to more easily use the package in offline environments with restricted internet access, particularly for Databricks Workspaces with extra layers of network security.
* Fixed bug for double-uploading of unreleased wheels in air-gapped setups ([#103](#103)). In this release, we have addressed a bug in the `upload_wheel_dependencies` method of the `WheelsV2` class, which caused double-uploading of unreleased wheels in air-gapped setups. This issue occurred due to the condition `if wheel.name == self._local_wheel.name` not being met, resulting in undefined behavior. We have introduced a cached property `_current_version` to tackle this bug for unreleased versions uploaded to air-gapped workspaces. We also added a new method, `upload_to_wsfs()`, that uploads files to the workspace file system (WSFS) in the integration test. This release also includes new tests to ensure that only the Databricks SDK is uploaded and that the number of installation files is correct. These changes have resolved the double-uploading issue, and the number of installation files, Databricks SDK, Blueprint, and version.json metadata are now uploaded correctly to WSFS.
@nfx nfx mentioned this pull request May 12, 2024
nfx added a commit that referenced this pull request May 12, 2024
* Added upstream wheel uploads for Databricks Workspaces without Public
Internet access
([#99](#99)). This
commit introduces a new feature for uploading upstream wheel
dependencies to Databricks Workspaces without Public Internet access. A
new flag has been added to upload functions, allowing users to include
or exclude dependencies in the download list. The `WheelsV2` class has
been updated with a new method, `upload_wheel_dependencies(prefixes)`,
which checks if each wheel's name starts with any of the provided
prefixes before uploading it to the Workspace File System (WSFS). This
feature also includes two new tests to verify the functionality of
uploading the main wheel package and dependent wheel packages,
optimizing downloads based on specific use cases. This enables users to
more easily use the package in offline environments with restricted
internet access, particularly for Databricks Workspaces with extra
layers of network security.
* Fixed bug for double-uploading of unreleased wheels in air-gapped
setups ([#103](#103)).
In this release, we have addressed a bug in the
`upload_wheel_dependencies` method of the `WheelsV2` class, which caused
double-uploading of unreleased wheels in air-gapped setups. This issue
occurred due to the condition `if wheel.name == self._local_wheel.name`
not being met, resulting in undefined behavior. We have introduced a
cached property `_current_version` to tackle this bug for unreleased
versions uploaded to air-gapped workspaces. We also added a new method,
`upload_to_wsfs()`, that uploads files to the workspace file system
(WSFS) in the integration test. This release also includes new tests to
ensure that only the Databricks SDK is uploaded and that the number of
installation files is correct. These changes have resolved the
double-uploading issue, and the number of installation files, Databricks
SDK, Blueprint, and version.json metadata are now uploaded correctly to
WSFS.
nkvuong added a commit to nkvuong/blueprint that referenced this pull request May 16, 2024
@nkvuong nkvuong mentioned this pull request May 21, 2024
nfx pushed a commit that referenced this pull request May 21, 2024
…comparing wheel uploads in development (#105)

Nightly is failing due to #103
- Made `ProductInfo.version` a `cached_property`, to avoid failure when
comparing wheel uploads in development
- Sorted upgrade scripts to ensure they are applied in semver order
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant