Add DEFAULT_AUGUR_RECURSION_LIMIT=10000 #1200
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1200 +/- ##
==========================================
+ Coverage 68.34% 68.37% +0.02%
==========================================
Files 64 64
Lines 6853 6852 -1
Branches 1663 1662 -1
==========================================
+ Hits 4684 4685 +1
+ Misses 1858 1857 -1
+ Partials 311 310 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
thanks, Jover. This makes sense to me. I think with the upgrade from TreeTime from 0.8.5 to >=0.9 the behavior of the polytomy resolution has changed slightly such that we are now often producing caterpillar like resolutions which leads to many more nesting and thus more recursion in newick output. |
|
I added the possibility to set the recursion limit in TreeTime a few days ago (and by default set it 10000). |
augur/__init__.py
Outdated
| recursion_limit = os.environ.get("AUGUR_RECURSION_LIMIT") | ||
| if recursion_limit: | ||
| sys.setrecursionlimit(int(recursion_limit)) | ||
| DEFAULT_AUGUR_RECURSION_LIMIT = 10000 | ||
| sys.setrecursionlimit(int(os.environ.get("AUGUR_RECURSION_LIMIT", DEFAULT_AUGUR_RECURSION_LIMIT))) |
There was a problem hiding this comment.
It's a small thing, but I think important to test the value of the env var, e.g. as if recursion_limit did.
AUGUR_RECURSION_LIMIT may be (intentionally or unintentionally) defined in the environment but have no value (e.g. be the empty string). We wouldn't want to call int(''). With .get("AUGUR_RECURSION_LIMIT", DEFAULT_AUGUR_RECURSION_LIMIT), keeping the if recursion_limit would let someone intentionally suppress the default (i.e. use Python's default). This is of dubious utility, though, so I'd probably roll the default and the value test into one thing instead, something like:
DEFAULT_AUGUR_RECURSION_LIMIT = 10000
sys.setrecursionlimit(int(os.environ.get("AUGUR_RECURSION_LIMIT") or DEFAULT_AUGUR_RECURSION_LIMIT))
There was a problem hiding this comment.
Ah, good catch! Updated to the suggested change in 1834540.
We've had to use the `AUGUR_RECURSION_LIMIT` environment variable to bump the recursion limit for SARS-CoV-2 builds¹, mpox builds², and our `nextstrain-jobs` definition in AWS Batch. From a general search of the envvar across GitHub³, 10,000 looks like the default that others use as well (although this may be influenced by our discussions⁴). ¹ nextstrain/ncov#690 ² nextstrain/mpox#150 ³ https://cs.github.com/?scopeName=All+repos&scope=&q=AUGUR_RECURSION_LIMIT%3D ⁴ #328
joverlee521
force-pushed
the
default-recursion-limit
branch
from
238b086
to
fab8c09
Compare
|
Just did some debugging with @miparedes on why an |
Description of proposed changes
We've had to use the
AUGUR_RECURSION_LIMITenvironment variable tobump the recursion limit for SARS-CoV-2 builds¹, mpox builds², and
our
nextstrain-jobsdefinition in AWS Batch. From a general search ofthe envvar across GitHub³, 10,000 looks like the default that others use
as well (although this may be influenced by our discussions⁴).
¹ nextstrain/ncov#690
² nextstrain/mpox#150
³ https://cs.github.com/?scopeName=All+repos&scope=&q=AUGUR_RECURSION_LIMIT%3D
⁴ #328
Testing
What steps should be taken to test the changes you've proposed?
If you added or changed behavior in the codebase, did you update the tests, or do you need help with this?
Checklist