fix: list-like args passed as string #5464
Conversation
github-actions
bot
added
size/S
area/core
Codecov Report
@@ Coverage Diff @@
## master #5464 +/- ##
==========================================
- Coverage 87.53% 87.52% -0.01%
==========================================
Files 121 121
Lines 9648 9645 -3
==========================================
- Hits 8445 8442 -3
Misses 1203 1203
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
github-actions
bot
added
size/M
area/helper
| # set_pod_parser returns a parser for worker runtime, which expects list of ports (because external executors | ||
| # can provide multiple ports and hosts). However this parser is not compatible with ContainerPod, Pod and worker runtime. | ||
| # Should we add a seperate parser for Pod? |
There was a problem hiding this comment.
@JoanFM you might need to check this
In my opinion, this is fine for now. We don't expect people to use the pod parser on the Worker runtime or Pod anyways
There was a problem hiding this comment.
what does it mean is not compatible wirh ContainerPod why not?
There was a problem hiding this comment.
because set_pod_parser generates args where port and host are lists.
This works fine with Flow and Deployment.
In the deployment layer, the lists are transformed to single values (for each shard and replica). The Pod and Worker runtime therefore expect single values. That's why, set_pod_parser cannot be used with Pod and worker runtime
tests/unit/orchestrate/flow/flow-construct/test_flow_yaml_parser.py
Outdated
Show resolved
Hide resolved
tests/integration/distributed-replicas/test_distributed_replicas.py
Outdated
Show resolved
Hide resolved
tests/integration/distributed-replicas/test_distributed_replicas.py
Outdated
Show resolved
Hide resolved
|
@alaeddine-13 @AnneYang720 please review test not workinf |
|
📝 Docs are deployed on https://fix-list-args--jina-docs.netlify.app 🎉 |
There was a problem hiding this comment.
I only have one doubt in the code. LGTM otherwise @alaeddine-13 @AnneYang720
| @@ -318,19 +323,6 @@ def update_pod_args(self): | |||
| else: | |||
| self.pod_args = self._parse_args(self.args) | |||
|
|
|||
| if self.external: | |||
There was a problem hiding this comment.
why is this logic not needed anymore?
There was a problem hiding this comment.
we decided to move it here, so that all setting logic is in one place
Goals:
resolves bug: list-like args passed as string #5352
parser can receive single value, str with "," and list of str
Flow can reveice list of values
Deployment can accept list of values + remove parsing logic
add tests
check and update documentation. See guide and ask the team.