Add an environment variable to disable ort session cache if necessary | fix(atenlib)#685
Add an environment variable to disable ort session cache if necessary | fix(atenlib)#685fatcat-z merged 6 commits intomicrosoft:mainfrom fatcat-z:remove_cache
Conversation
fatcat-z
added
the
module: torchlib
|
How about removing the cache? Any cons? |
@gramalingam , any comments about adding this cache in the beginning? |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| from onnxscript import autocast, irbuilder, onnx_opset, tensor, utils, values | ||
| from onnxscript._internal import param_manipulation | ||
| from onnxscript._internal import feature_switch, param_manipulation |
Check notice
Code scanning / CodeQL
Cyclic import
| import os | ||
|
|
||
| # By default: Enable | ||
| CACHE_ORT_SESSIONS: bool = os.getenv("CACHE_ORT_SESSIONS", "1") != "0" |
There was a problem hiding this comment.
| CACHE_ORT_SESSIONS: bool = os.getenv("CACHE_ORT_SESSIONS", "1") != "0" | |
| cache_ort_sessions: bool = os.getenv("CACHE_ORT_SESSIONS", "1") != "0" |
sorry - I changed my mind because it looks like it is a flag that can be changed in runtime (not constant), so snake case instead?
There was a problem hiding this comment.
Is this in _internal by design? I can see arguments both ways. It should ideally be exposed to the user, but if we think this API will change, I can see a point in leaving it here.
There was a problem hiding this comment.
Is this in
_internalby design? I can see arguments both ways. It should ideally be exposed to the user, but if we think this API will change, I can see a point in leaving it here.
My opinion is: Such switches should not be exposed to users calling onnx-script ops or torch_libs to construct a model. Because these switches control some internal features of onnx-script only. These internal features are not necessary to be known by those users.
There was a problem hiding this comment.
sorry - I changed my mind because it looks like it is a flag that can be changed in runtime (not constant), so snake case instead?
I think a variable in upper case is more like a constant which should not be changed in runtime. Isn't it? And I think this is aligned with the assumption that these switches are designed to be controlled by the environment variable only, so we don't need to change them to snake case.
There was a problem hiding this comment.
consider flags.py as in "feature flags"
There was a problem hiding this comment.
In my past projects, such things which determines if a feature is enable or not were named like "xxxx_switch", so I followed them.
In addition, I think 'switch' implies this can control something while 'flag' is more like a static label.
justinchuby
left a comment
justinchuby
left a comment
There was a problem hiding this comment.
Thanks! Last round of minor suggestions.
3 participants
Try to disable ort session cache to avoid Windows memory exception caused by CI tests.
Fix #614