Update Python support #384
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,21 +19,24 @@ jobs: | |
| - name: Checkout Code Repository | ||
| uses: actions/checkout@v3 | ||
|
|
||
| - name: Set up Python 3.13 | ||
| uses: actions/setup-python@v4 | ||
| with: | ||
| python-version: "3.13" | ||
|
|
||
| # Run all pre-commit hooks on all the files. | ||
| # Getting only staged files can be tricky in case a new PR is opened | ||
| # since the action is run on a branch in detached head state. | ||
| # This is the equivalent of running "pre-commit run --all-files" locally. | ||
| # If you commit with the `--no-verify` flag, this check may fail. | ||
| - name: Install and Run Pre-commit | ||
| uses: pre-commit/[email protected].1 | ||
|
|
||
| build: | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| python-version: ["3.11", "3.12", "3.13"] | ||
| defaults: | ||
| run: | ||
| shell: bash -l {0} | ||
|
|
@@ -44,11 +47,11 @@ jobs: | |
| - name: Cache Conda | ||
| uses: actions/cache@v3 | ||
| env: | ||
| CACHE_NUMBER: 1 # Increment this to invalidate cache | ||
| with: | ||
| path: ~/conda_pkgs_dir | ||
| key: ${{ runner.os }}-conda-${{ env.CACHE_NUMBER }}-${{ | ||
| hashFiles('conda/dev.yml') }}-python${{ matrix.python-version }} | ||
|
|
||
| - name: Build Conda Environment | ||
| uses: conda-incubator/setup-miniconda@v3 | ||
|
|
@@ -57,13 +60,27 @@ jobs: | |
| miniforge-variant: Miniforge3 | ||
| miniforge-version: latest | ||
| environment-file: conda/dev.yml | ||
| channel-priority: flexible # Changed from strict to flexible | ||
| auto-update-conda: true | ||
| python-version: ${{ matrix.python-version }} | ||
| channels: conda-forge | ||
| use-only-tar-bz2: true | ||
|
|
||
| - name: Verify Environment and Fix Dependencies | ||
| run: | | ||
| conda info | ||
| conda list | ||
| # Ensure we have the right Python version | ||
| python --version | ||
| # Fix pip issues for Python 3.12+ | ||
| if [[ "${{ matrix.python-version }}" == "3.12" ]] || [[ "${{ matrix.python-version }}" == "3.13" ]]; then | ||
| python -m ensurepip --upgrade || true | ||
| python -m pip install --upgrade --force-reinstall pip setuptools wheel | ||
| fi | ||
|
|
||
| - name: Install `zstash` Package | ||
| run: | | ||
| python -m pip install . | ||
|
|
||
| - name: Run Tests | ||
| run: | | ||
|
|
@@ -77,7 +94,7 @@ jobs: | |
| defaults: | ||
| run: | ||
| shell: bash -l {0} | ||
| timeout-minutes: 10 # Increased timeout for docs | ||
| steps: | ||
| - uses: actions/checkout@v3 | ||
| with: | ||
|
|
@@ -87,11 +104,11 @@ jobs: | |
| - name: Cache Conda | ||
| uses: actions/cache@v3 | ||
| env: | ||
| CACHE_NUMBER: 1 # Match the build job cache number | ||
| with: | ||
| path: ~/conda_pkgs_dir | ||
| key: ${{ runner.os }}-conda-${{ env.CACHE_NUMBER }}-${{ | ||
| hashFiles('conda/dev.yml') }}-docs | ||
|
|
||
| - name: Build Conda Environment | ||
| uses: conda-incubator/setup-miniconda@v3 | ||
|
|
@@ -100,8 +117,9 @@ jobs: | |
| miniforge-variant: Miniforge3 | ||
| miniforge-version: latest | ||
| environment-file: conda/dev.yml | ||
| channel-priority: flexible # Changed from strict to flexible | ||
| auto-update-conda: true | ||
| python-version: "3.13" # Use stable Python version for docs | ||
|
|
||
| # sphinx-multiversion allows for version docs. | ||
| - name: Build Sphinx Docs | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,31 +1,30 @@ | ||
| name: zstash_dev | ||
| channels: | ||
| - conda-forge | ||
| dependencies: | ||
| # Base | ||
| # ================= | ||
| - pip | ||
| - python >=3.11,<3.14 | ||
| - sqlite | ||
| - six >=1.16.0 | ||
| - globus-sdk >=3.15.0 | ||
| # Developer Tools | ||
| # ================= | ||
| # If versions are updated, also update 'rev' in `.pre-commit.config.yaml` | ||
| - black ==25.1.0 | ||
| - flake8 ==7.3.0 | ||
| - isort ==6.0.1 | ||
| - mypy ==1.18.2 | ||
| - pre-commit ==4.3.0 | ||
| - tbump >=6.9.0 | ||
| # Documentation | ||
| # ================= | ||
| # If versions are updated, also update in `.github/workflows/workflow.yml` | ||
| - jinja2 <3.1 | ||
| - sphinx >=5.2.0 | ||
| - sphinx-multiversion >=0.2.4 | ||
| - sphinx_rtd_theme >=1.0.0 | ||
| # Need to pin docutils because 0.17 has a bug with unordered lists | ||
| # https://github.com/readthedocs/sphinx_rtd_theme/issues/1115 | ||
| - docutils >=0.16,<0.17 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,6 @@ | |
| author_email="[email protected], [email protected], [email protected]", | ||
| description="Long term HPSS archiving software for E3SM", | ||
| packages=find_packages(include=["zstash", "zstash.*"]), | ||
| python_requires=">=3.11,<3.14", | ||
| entry_points={"console_scripts": ["zstash=zstash.main:main"]}, | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,6 @@ | |
| from typing import List, Optional, Tuple | ||
|
|
||
| import _hashlib | ||
|
|
||
| from .hpss import hpss_put | ||
| from .settings import BLOCK_SIZE, TupleFilesRowNoId, TupleTarsRowNoId, config, logger | ||
|
|
@@ -20,7 +19,7 @@ | |
| # Minimum output file object | ||
| class HashIO(object): | ||
| def __init__(self, name: str, mode: str, do_hash: bool): | ||
| self.f = open(name, mode) | ||
| self.hash: Optional[_hashlib.HASH] | ||
| if do_hash: | ||
| self.hash = hashlib.md5() | ||
|
|
@@ -281,43 +280,29 @@ def add_file( | |
| # Change the size of any hardlinks from 0 to the size of the actual file | ||
| if tarinfo.islnk(): | ||
| tarinfo.size = os.path.getsize(file_name) | ||
|
|
||
| # Add the file to the tar | ||
| if (tarinfo.isfile() or tarinfo.islnk()) and tarinfo.size > 0: | ||
| with open(file_name, "rb") as fileobj: | ||
| tar.addfile(tarinfo, fileobj) | ||
| else: | ||
| tar.addfile(tarinfo) | ||
|
||
|
|
||
| md5: Optional[str] = None | ||
| # Only add files or hardlinks. | ||
| # (So don't add directories or softlinks.) | ||
| if tarinfo.isfile() or tarinfo.islnk(): | ||
| f = open(file_name, "rb") | ||
| hash_md5: _hashlib.HASH = hashlib.md5() | ||
|
|
||
| while True: | ||
|
Comment on lines
-304
to
-317
Collaborator
Author
There was a problem hiding this comment. It appears we don't actually need to be tracking/updating
Collaborator
Author
There was a problem hiding this comment. After following # Add file to tar archive while computing its hash
# Return file offset (in tar archive), size and md5 hash
def addfile(tar, file):
offset = tar.offset
tarinfo = tar.gettarinfo(file)
tar.addfile(tarinfo)
if tarinfo.isfile():
f = open(file, "rb")
hash_md5 = hashlib.md5()
while True:
s = f.read(BLOCK_SIZE)
if len(s) > 0:
tar.fileobj.write(s)
hash_md5.update(s)
if len(s) < BLOCK_SIZE:
blocks, remainder = divmod(tarinfo.size, tarfile.BLOCKSIZE)
if remainder > 0:
tar.fileobj.write(tarfile.NUL *
(tarfile.BLOCKSIZE - remainder))
blocks += 1
tar.offset += blocks * tarfile.BLOCKSIZE
break
f.close()
md5 = hash_md5.hexdigest()
else:
md5 = None
size = tarinfo.size
mtime = datetime.utcfromtimestamp(tarinfo.mtime)
return offset, size, mtime, md5So, I just want to make sure the new
Contributor
There was a problem hiding this comment. My reading of the documentation is that yes, the tar file is updated already with the The advantage of the original implementation was probably that each file got opened once only and was added to the archive at the same time as computing its md5 hash. The new code reopens the file to create the md5 hash but I think that's fine (and seemingly unavoidable with the new API).
Collaborator
There was a problem hiding this comment. I'm sorry, but I cannot approve this change. The ability to stream data to tar files and compute hashes at the same time was a key functionality of zstash. Computation of md5 hashes is expensive, removing this functionality will have a significant performance impact.
Collaborator
Author
There was a problem hiding this comment. Claude's feedback:If the old implementation breaks on Python 3.13 (likely due to changes in how Options to Consider1. Accept the performance hit (pragmatic choice)
2. Implement a hybrid solution (best of both worlds)def add_file(tar, file_name, follow_symlinks):
offset = tar.offset
tarinfo = tar.gettarinfo(file_name)
if tarinfo.islnk():
tarinfo.size = os.path.getsize(file_name)
md5 = None
# For files/hardlinks, stream data while computing hash
if (tarinfo.isfile() or tarinfo.islnk()) and tarinfo.size > 0:
hash_md5 = hashlib.md5()
# Create a wrapper that computes hash while data passes through
class HashingFileWrapper:
def __init__(self, fileobj, hasher):
self.fileobj = fileobj
self.hasher = hasher
def read(self, size=-1):
data = self.fileobj.read(size)
if data:
self.hasher.update(data)
return data
with open(file_name, "rb") as f:
wrapper = HashingFileWrapper(f, hash_md5)
tar.addfile(tarinfo, wrapper)
md5 = hash_md5.hexdigest()
else:
tar.addfile(tarinfo)
size = tarinfo.size
mtime = datetime.utcfromtimestamp(tarinfo.mtime)
return offset, size, mtime, md5This wrapper approach:
3. Version-specific implementationsUse the old code for Python <3.13 and new code for 3.13+, but this adds maintenance burden. My RecommendationTry the hybrid solution first (Option 2). It should give you Python 3.13 compatibility while preserving the streaming performance that Reviewer 2 correctly identified as important. If If the wrapper approach doesn't work for some reason, then you'll need to accept the performance regression as the cost of Python 3.13 support, but at least you'll have tried to preserve the optimization.
Contributor
There was a problem hiding this comment. @golaz, I will veto any solution that does not allow us to advance to the latest stable python (python 3.13), since this is something I have been fighting to have happen in Unified for years and it has finally become possible because of dropping CDAT. It seems like zstash has not been getting the TLC it has needed over the years to remain compatible with modern python and that's pretty disappointing. I think @forsyth2 is spread too thin and no one else has stepped up. Goodness knows I'm spread too thin. Hopefully, we can get things there for Unified, since testing should start on Monday. Hopefully, we can also find a longer term solution for zstash to have a "champion".
Collaborator
There was a problem hiding this comment. Sorry I haven’t followed this thread too closely. Which Python version is causing the incompatibility with zstash? I think it’s a solid achievement that we could update from 3.10 (now end-of-life) to 3.11 (or higher, if our tools are ready) for Unified with all the work we put in to migrate from CDAT. In this case, it seems zstash isn’t ready yet, and we are time-constrained to update it with thorough testing. Can we target Python 3.11 or 3.12 (depending on zstash readiness) for this upcoming unified? Both are still widely used, and 3.11 will continue receiving security updates until October 2027. Again, for a tool like E3SM-unified, I believe we should continue prioritizing maximum compatibility.
Contributor
There was a problem hiding this comment. AAAAAAaargh!! Okay, I think it would be important to find out if zstash works with python 3.12 and without requiring these changes. If we only support python 3.11 in zstash and Unified, that's not good. That's the state we just got out of with dropping CDAT. I also never want this to happen again with a future Unified. All our software must, must, must support the 3 latest stable versions of python at all times. This is just necessary for our ecosystem to function well. Otherwise, the neglected tools really drag down the tools that are trying to take advantage of more modern capabilities.
Collaborator
Author
There was a problem hiding this comment.
I've implemented this in the latest commit: cc37237. It required an additional change to handle empty files. It now passes the GitHub Actions for 3.11, 3.12, and 3.13. I believe this means this PR should be good to merge. @xylar @golaz Can you please re-confirm based on this latest commit?
Yes, I agree with all this. After the Unified testing period, perhaps it would be useful to have a team discussion about this. The problem is certainly not a lack of ideas re: code maintainability (e.g., removing all those global variables or improving the testing).
3.13
It does. I only ran into errors when I started matrix-testing on 3.13.
This is good to keep in mind, thanks.
Collaborator
There was a problem hiding this comment.
I agree with @chengzhuzhang that maximizing compatibility is essential, and with @xylar that we must support the latest stable Python versions (currently 3.11, 3.12, 3.13). Falling behind with Python support (mainly due to CDAT) means we’re not truly maximizing compatibility and leaves us playing catch-up with the broader ecosystem. Our sub-dependencies (e.g., |
||
| data = f.read(BLOCK_SIZE) | ||
| if len(data) > 0: | ||
| hash_md5.update(data) | ||
| if len(data) < BLOCK_SIZE: | ||
| break | ||
| md5 = hash_md5.hexdigest() | ||
| f.close() | ||
| size: int = tarinfo.size | ||
| mtime: datetime = datetime.utcfromtimestamp(tarinfo.mtime) | ||
| return offset, size, mtime, md5 | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomvothecoder How do we determine the constraints (
=,>=, etc.)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is based on the conda-forge recipe and compatibility with other packages. If things break with new package versions, that suggests updating the constraints accordingly.