Skip to content

Conversation

@eduardoChaucaGallegos
Copy link
Contributor

@eduardoChaucaGallegos eduardoChaucaGallegos commented Dec 31, 2024

Description

This pull request introduces a major overhaul to how third-party Python dependencies are bundled, loaded, and managed in the project. It centralizes dependency management using versioned pkgs.zip archives, implements a dynamic import system for the tank_vendor namespace, and updates documentation to reflect the new workflow. Additionally, it removes legacy code and files that are no longer compatible with the new approach.

Key changes include:

Dependency Management Overhaul

  • Introduced a new system where dependencies for each supported Python version are bundled in a pkgs.zip archive, located in the requirements/<version>/ directories. This ensures consistent, reproducible environments and simplifies dependency updates.
  • Updated documentation in developer/README.md to describe the new dependency workflow, including how to update, freeze, and bundle packages, as well as how to perform CVE checks using frozen_requirements.txt. [1] [2]

Dynamic Import System for tank_vendor

  • Rewrote python/tank_vendor/__init__.py to automatically discover, load, and alias all packages from the appropriate pkgs.zip for the running Python version. It installs a custom import hook to support lazy loading and seamless aliasing for tank_vendor.* imports, including submodules.
  • Added logic to patch specific packages (like shotgun_api3) to work correctly when loaded from a ZIP archive, e.g., handling SSL certificate files.
  • Ensured backward compatibility for environments that do not use pkgs.zip by falling back to pip-installed dependencies and still supporting the tank_vendor namespace.

Codebase Cleanup and Compatibility

  • Removed the legacy upgrade_pyyaml.py script, as the new workflow supersedes the need for manual upgrades of individual dependencies.
  • Deleted legacy files from the vendored packaging library that are now managed via the new bundling system. (python/tank_vendor/packaging/__init__.py, python/tank_vendor/packaging/_structures.py) [1] [2]
  • Updated the import handler in python/tank/bootstrap/import_handler.py to correctly delegate loading of modules from ZIP files to the appropriate import mechanism, ensuring compatibility with the new system.

References:
[1] [2] [3] [4] [5] [6] [7]

Resolves #914

@julien-lang julien-lang requested a review from a team January 6, 2025 16:29
@eduardoChaucaGallegos eduardoChaucaGallegos marked this pull request as draft January 28, 2025 17:30
@codecov
Copy link

codecov bot commented Feb 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.79%. Comparing base (67de82a) to head (c92a72b).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #996   +/-   ##
=======================================
  Coverage   79.79%   79.79%           
=======================================
  Files         198      198           
  Lines       20770    20760   -10     
=======================================
- Hits        16573    16566    -7     
+ Misses       4197     4194    -3     

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

Copy link
Contributor

@carlos-villavicencio-adsk carlos-villavicencio-adsk left a comment

Choose a reason for hiding this comment

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

LGTM. Good stuff!

@triley13
Copy link

ETA on this?

@julien-lang
Copy link
Member

Hi @triley13, sorry, no, we don't have an ETA. We had to postpone this PR because of new priorities. We still plan to merge it eventually but not in the next few weeks.

Is this something you need? Feel free to share how this change could help you.

@julien-lang
Copy link
Member

Replaces #914

@triley13
Copy link

Hi @triley13, sorry, no, we don't have an ETA. We had to postpone this PR because of new priorities. We still plan to merge it eventually but not in the next few weeks.

Is this something you need? Feel free to share how this change could help you.

Right now we use Rez to manage our internal libraries. This helps to keep our repo clean of duplicating libraries and implementing CICD to package the site-packages with our internal libs. With this, we can pull from our py registry (github), to manage our library injections.

# ZIP import handler (like zipimport or TankVendorMetaFinder) handle it.
# This is common for tank_vendor packages that come from pkgs.zip.
if package_path[0] and ".zip" in package_path[0]:
return None
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return None
return

packaging==24.0
pyyaml==5.4.1
ruamel.yaml==0.18.13
shotgun_api3==3.9.0
Copy link
Member

Choose a reason for hiding this comment

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

Why is this one not 3.9.1 like the other ones?

Copy link
Member

Choose a reason for hiding this comment

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

Can we have this script either in the requirements folder or the developer folder instead?

Comment on lines +261 to +266
# If pkgs.zip is not found, assume pip-style installation where dependencies
# are installed directly in the Python environment. In this case, we still
# install the import hook to enable tank_vendor.* aliasing for compatibility.
if not _pkgs_zip_valid:
# Install import hook even without pkgs.zip for pip installations
_install_import_hook()
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why we want to handle this case.

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.

5 participants