Issue 489: Fix the links #520
Conversation
ada333
requested review from
hmtosi and
jesuino
and removed request for
jesuino
| 'kfp.get_ui_host', | ||
| ); | ||
| } catch (error) { | ||
| console.error('Failed to retrieve KFP UI host', error); |
There was a problem hiding this comment.
Would you please give a little more detail on how user can set up this value?
There was a problem hiding this comment.
When the value is not set the backend function returns null and this error is not triggered. I am trying to think of the best solution for this - how to let the user know that he needs to set up the env var KF_PIPELINES_UI_ENDPOINT and that default is localhost:8080
There was a problem hiding this comment.
I agree, we need to determine/document how to set up the env variable KF_PIPELINES_UI_ENDPOINT. I was not able to determine this in the code. However, I was able to reassign the default to localhost:8081 and have that work successfully.
| return '#'; | ||
| } | ||
| const link = `${window.location.origin}/_/pipeline/#/runs/details/${run.id}`; | ||
| const base = props.kfpUiHost || 'http://localhost:8080'; |
There was a problem hiding this comment.
I would make the default URL a constant so it will be easier to modify in future.
const DEFAULT_UI_URL = 'http://localhost:8080'
...
const base = props.kfpUiHost || DEFAULT_UI_URL;
There was a problem hiding this comment.
Adam has made the change and the constant is easy to modify
hmtosi
left a comment
hmtosi
left a comment
There was a problem hiding this comment.
Testing
I tested this on my default environment (automatically running kfp on port 8080, no code changes), and it works well!
I also made the following changes to try running it on a different port:
/labextension/src/widgets/deploys-progress/DeployProgress.tsx:
- line 34:
const DEFAULT_UI_URL = 'http://localhost:8081';-> setting the default to 8081 - line 61:
const base = DEFAULT_UI_URL-> forcing the upload link to be the default url - line 72:
const base = DEFAULT_UI_URL-> forcing the run link to be the default url
kubectl port-forward -n kubeflow svc/ml-pipeline-ui 8081:80-> running kfp ui on port8081
After the changes, pipelines still run successfully, and the links still work. They also work if just the default is changed (line 34).
Review
This fix works well - the links are no longer broken, and a default port is set in the code, which can be easily adjusted. It is no longer a blocker in my opinion.
However, I think after the donation we could set up an env file in the root directory that keeps env variables, and lets the user assign them there. That way, a users's env information is not hardcoded or uploaded to github, and they only need to edit a .txt file (not the actual code). This would take some code refactoring.
Also, I do not understand how to adjust KF_PIPELINES_UI_ENDPOINT. I see that this and other environment variables are assigned in the venv (.venv/lib/python3.10/site-packages/kfp/client/client.py). I think we should only give the user one place to adjust this (either KF_PIPELINES_UI_ENDPOINT or DEFAULT_UI_URL). And when the user does assign this variable, it would be better for it to be in an ignored file, so user credentials are not accidentally merged to github.
Conclusion
This is great work, and I think it is good enough for now.
After the donation, we could think about refactoring the code so that users can set up an env file in their root directory, which
a) holds all the environment variables
b) allows the user to easily edit environment variables
c) protects user-specific information from getting to github
3 participants
Right now it is a draft because the 8080 is hardcoded and I wasnt able to get the value any other way.
This PR should fix the links when running a notebook in Kale.
Right now the localhost:8080 is hardcoded because calling .host or .uihost hasnt worked for me.
Also I added a better error log when opening a compiled path: