Skip to content

Improve environmental variable caching #2101

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

stxue1
Copy link
Contributor

@stxue1 stxue1 commented Feb 19, 2025

PR to support environmental variable caching as mentioned here: DataBiosphere/toil#5187 (comment)

The structure of the preserve_environment code is roughly copied from a related function:

cwltool/cwltool/job.py

Lines 486 to 497 in 1b56338

if runtimeContext.preserve_entire_environment:
self._preserve_environment_on_containers_warning()
env.update(os.environ)
elif runtimeContext.preserve_environment:
self._preserve_environment_on_containers_warning(runtimeContext.preserve_environment)
for key in runtimeContext.preserve_environment:
try:
env[key] = os.environ[key]
except KeyError:
_logger.warning(
f"Attempting to preserve environment variable {key!r} which is not present"
)

Copy link

codecov bot commented Feb 19, 2025

Codecov Report

Attention: Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.82%. Comparing base (346f421) to head (85e7aae).

Files with missing lines Patch % Lines
cwltool/command_line_tool.py 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2101      +/-   ##
==========================================
+ Coverage   84.75%   84.82%   +0.07%     
==========================================
  Files          46       46              
  Lines        8368     8381      +13     
  Branches     1962     1966       +4     
==========================================
+ Hits         7092     7109      +17     
+ Misses        808      804       -4     
  Partials      468      468              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mr-c mr-c changed the title Add environmental variable caching Improve environmental variable caching Feb 20, 2025
Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

Thank you @stxue1 ; this will need some extra tests added to confirm your fixes

Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

Thanks for the extra tests! I see that they motivated some code changes :-)

@mr-c mr-c force-pushed the env_var_caching branch 2 times, most recently from 37aa2ed to 0a2475c Compare March 2, 2025 14:58
@mr-c mr-c force-pushed the env_var_caching branch from 0a2475c to fbc23fd Compare April 23, 2025 14:47
mr-c and others added 2 commits May 20, 2025 14:47
Example: `tox -e py311-unit -- --lf --pdb`
Especially with regards to overrides and
`--preserve{-entire,}-environement`

Added more tests for environment variable caching

Co-authored-by: Michael R. Crusoe <mrc@commonwl.org>
@mr-c mr-c force-pushed the env_var_caching branch from fbc23fd to 85e7aae Compare May 20, 2025 12:47
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants