SDK - Compiler - Validating Argo validator #3874
Conversation
|
This change is |
sdk/python/kfp/compiler/compiler.py
Outdated
| try: | ||
| has_working_argo_lint = _run_argo_lint('') | ||
| except: | ||
| pass |
There was a problem hiding this comment.
Shall we log a warning message here? So it doesn't feel lost for end users why sometimes argo lint is there, but sometimes not
There was a problem hiding this comment.
I think most our users do not have argo CLI installed. I'm not sure we want to show warnings for those users.
WDYT?
There was a problem hiding this comment.
Shall we move this check after checking if argo exists in PATH? That seems to solve this case.
There was a problem hiding this comment.
Also, if linting with argo is a useful feature (sounds to me, it's very useful). If argo is not in path, can we log an info message suggesting users to do so?
There was a problem hiding this comment.
Shall we move this check after checking if argo exists in PATH? That seems to solve this case.
We have some bug reports from people who either have different programs called argo in path or have recent versions of argo that cannot work inside container. In both cases the executable exists in path, but is not suitable.
This is why we first validate it on empty input (maybe we should use a small pipeline).
There was a problem hiding this comment.
Yes, I think moving this check after checking argo in PATH is a better UX for the cases you mentioned.
e.g. when user has a later argo cli version, they will see an info message that kfp cannot use the argo cli for linting, because "xxx".
Then they know:
- kfp sdk can use argo cli to lint argo workflow (that's a really useful feature)
- how they can enable the feature (by switching to an argo cli of suitable version)
- if they don't want use this feature, just ignore this info message, it won't block execution
|
/retest |
1 similar comment
|
/retest |
|
Thanks! This looks great /lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ark-kun The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* SDK - Compiler - Validating Argo validator * Added warning if argo is available, but not working
Fixes #3859