fix: parsing of env vars of the form '{ var }' #4229
Conversation
github-actions
bot
added
size/S
area/core
Codecov Report
@@ Coverage Diff @@
## master #4229 +/- ##
==========================================
- Coverage 87.40% 86.86% -0.55%
==========================================
Files 114 114
Lines 8284 8306 +22
==========================================
- Hits 7241 7215 -26
- Misses 1043 1091 +48
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Latency summaryCurrent PR yields:
Breakdown
Backed by latency-tracking. Further commits will update this comment. |
samsja
requested review from
jacobowitz and
JoanFM
and removed request for
JoanFM
fix: WIP cvoer all jaml parsing case correclty
samsja
force-pushed
the
fix-env-vars-4059
branch
from
9e1bbe4
to
320bb1c
Compare
JoanFM
changed the title
|
|
||
| Note the mandatory spaces before and after the variable denotation. | ||
|
|
||
| Context variables can be accessed using the following syntax: |
There was a problem hiding this comment.
What is a context var? Isn't this the same
${{ env.var }}
# and
${{ var }}
```?
There was a problem hiding this comment.
No, the latter refers exclusively to context variables, which can be passed to Flow.load_config(), Executor.load_config(), Jaml.parse() as a dict. Slightly more info is below. We didn't know about these either until we found them during testing.
| @@ -494,8 +600,8 @@ def load_config( | |||
| :class:`BaseExecutor`, :class:`BaseDriver` and all their subclasses. | |||
|
|
|||
| Support substitutions in YAML: | |||
| - Environment variables: `${{ENV.VAR}}` (recommended), ``${{VAR}}``. | |||
| - Context dict (``context``): ``${{VAR}}``(recommended). | |||
| - Environment variables: ``${{ ENV.VAR }}`` (recommended), ``$VAR`` (deprecated). | |||
There was a problem hiding this comment.
If it's deprecated, why are we keeping it?
There was a problem hiding this comment.
General question. can be raised in slack or in an alignment. How long are we going to keep it around?
There was a problem hiding this comment.
We are trying to align with GitHub actions syntax, in which both work, but the first version is encouraged. Han told us to deprecate the second version, my guess is it will stick around for as long as it does within github actions.
|
|
||
| To substitute the value based on a dict, | ||
| These context variables are passed as a dict: |
There was a problem hiding this comment.
I've always passed env vars directly, without the need to ENV. prefix. Will it not work from now on? In what situation would you want context variables and how would you pass them to a Flow?
There was a problem hiding this comment.
Passing env vars like this: ${{ var }} should never have worked in the first place, we believe, and does not work anymore. So now you either do ${{ env.var }} or $var; the latter is discouraged but should work perfectly fine. You should be able to pass a dict containing context variables to Flow.load_config(context={'context_var' : 1}) . Not sure what the use cases for context vars are, though.
|
The yaml parsing now strictly respect the github actions rules |
| @@ -49,17 +82,45 @@ class DummyClass: | |||
| An expression can be any combination of literal values, references to a context, or functions. | |||
| You can combine literals, context references, and functions using operators. | |||
|
|
|||
| You need to use specific syntax to tell Jina to evaluate an expression rather than treat it as a string. | |||
| You need to use specific syntax to tell Jina to evaluate an expression rather than treat it as a string, | |||
| which is based on GitHub actions syntax, and looks like this: | |||
There was a problem hiding this comment.
is there a link to Github syntax explained?
No description provided.