flytekit icon indicating copy to clipboard operation
flytekit copied to clipboard

Fix source code link

Open pingsutw opened this issue 1 year ago • 3 comments

Tracking issue

NA

Why are the changes needed?

We only show the repo link in the UI, which is not helpful. It would be great if we could display the link to the file.

What changes were proposed in this pull request?

  • Updated the source link (<github_repo_link> => <github_repo_link>/blob/<git_sha>/<file_path>)
  • Only show the link if the file has been pushed

How was this patch tested?

pyflyte run ...
from datetime import datetime

from flytekit import task, workflow


@task(cache=True, cache_version="1.0")
def t1(x: int) -> datetime:
    print(x)
    return datetime.now()


@workflow
def wf():
    """
    Dummy workflow for functional test.
    """
    t1(x=10)

Setup process

Screenshots

Screenshot 2024-08-30 at 12 24 15 PM

Check all the applicable boxes

  • [x] I updated the documentation accordingly.
  • [x] All new and existing tests passed.
  • [x] All commits are signed-off.

Related PRs

NA

Docs link

NA

pingsutw avatar Jul 25 '24 20:07 pingsutw

@thomasjpfan @cosmicBboy, I'd like to get your thoughts here. Showing the source code link on the UI is quite useful, but I found a UX issue.

currently, the link is <github_repo_link>/blob/<git_sha>/<file_path>

Problems:

The link doesn't work if users haven't pushed the code yet. (I think this is a common use case. People always register and run the task first, and then push their code.)

And the link is immutable, so they have to register a new version of the task/workflow again to see the link after they push their code.

Proposed solution

If users haven't pushed the code yet

  1. Should we just not show the link?
  2. or show the link of the current or master branch. (<github_repo_link>/blob/master/<file_path>)
  3. not show the link, but show a warning message while registering
  4. only show the link of the git repo. like https://github.com/unionai/unionai-examples
  5. Instead of showing the link, display the entire code on the UI. (maybe too much)

Screenshot 2024-08-30 at 12 24 15 PM

pingsutw avatar Aug 30 '24 19:08 pingsutw

The link doesn't work if users haven't pushed the code yet. (I think this is a common use case. People always register and run the task first, and then push their code.)

  • During registration, check if the URL 404s. If it exists, then include the link.
  • If it does not exists, show the remote URL and state that code was not committed. If we want to be clever, show the URL to the branch but still state that the code as not commited.

I rather not warn since this is a common use case and warning is too noisy.

It'll be great to link to the code in the UI, maybe another VSCode integration. In this case it's using VSCode to "explore your files".


As for the UI, I would show a link and not a full URL. Also each version of the workflow should have it's own link, because the code could be different.

thomasjpfan avatar Aug 30 '24 20:08 thomasjpfan

Codecov Report

Attention: Patch coverage is 52.38095% with 30 lines in your changes missing coverage. Please review.

Project coverage is 55.46%. Comparing base (c7cfc27) to head (0121c48). Report is 7 commits behind head on master.

Files with missing lines Patch % Lines
flytekit/tools/translator.py 30.43% 15 Missing and 1 partial :warning:
flytekit/remote/remote.py 33.33% 8 Missing :warning:
flytekit/core/interface.py 66.66% 3 Missing and 1 partial :warning:
flytekit/core/workflow.py 66.66% 2 Missing :warning:
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #2609       +/-   ##
===========================================
- Coverage   78.68%   55.46%   -23.22%     
===========================================
  Files         278      280        +2     
  Lines       23280    24128      +848     
  Branches     3845     4099      +254     
===========================================
- Hits        18317    13383     -4934     
- Misses       4287    10252     +5965     
+ Partials      676      493      -183     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Sep 04 '24 07:09 codecov[bot]

Cleaning stale PRs. Please reopen if you wan to discuss this further.

eapolinario avatar Mar 06 '25 20:03 eapolinario