whole-tale / girder_wholetale

Girder plugin providing basic Whole Tale functionality
BSD 3-Clause "New" or "Revised" License
3 stars 5 forks source link

Git integration #430

Closed Xarthisius closed 3 years ago

Xarthisius commented 4 years ago

Rationale

Allows to either import a git repo to a workspace of an existing Tale, or import a remote git repo as a Tale (AinWT flow).

new API:

How to test?

  1. Checkout and run this branch locally
  2. Ensure dependencies are installed: docker exec -ti $(docker ps --filter=name=wt_girder -q) girder-install plugin plugins/wholetale
  3. t.b.a
codecov[bot] commented 4 years ago

Codecov Report

Merging #430 (238a921) into master (69548de) will increase coverage by 0.14%. The diff coverage is 94.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #430      +/-   ##
==========================================
+ Coverage   90.82%   90.97%   +0.14%     
==========================================
  Files          51       52       +1     
  Lines        3598     3699     +101     
==========================================
+ Hits         3268     3365      +97     
- Misses        330      334       +4     
Impacted Files Coverage Δ
server/rest/tale.py 86.00% <86.48%> (+0.03%) :arrow_up:
server/tasks/import_git_repo.py 97.43% <97.43%> (ø)
server/constants.py 88.63% <100.00%> (+0.54%) :arrow_up:
server/models/tale.py 97.15% <100.00%> (+0.08%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 69548de...238a921. Read the comment docs.

Xarthisius commented 4 years ago

@bodom0015 do you think we should wire this with https://github.com/whole-tale/ngx-dashboard/pull/54 and ask people to review both of them at the same time?

bodom0015 commented 3 years ago

That seems sensible, since they're linked.. will include that in my test case :+1:

bodom0015 commented 3 years ago

Maybe I'm doing something wrong here, but creating a Tale with the /tale/import endpoint doesn't seem to add anything new to my Tale Workspace after importing when git=true. I was expecting an error somewhere about how helloworld4 isn't a real URL, but I'm not seeing anything in the celery_worker or girder logs about a failed job or failed clone.

I can try again with a real URL if that might be an issue. Since this runs as a local job, would I be looking at the Girder container for the log output?

My request: POST /api/v1/tale/import?git=true&url=helloworld4&taleKwargs=%7B%22title%22%3A%22helloworld4%22%7D&spawn=true&lookupKwargs=%7B%7D&imageId=5f8878fc263f3a13f9a12b82&asTale=false

Also noticed that this endpoint results in a null description on the Tale. I think our default for the other endpoint is ### Please provide a description for your Tale or something similar.

Resulting Tale:

  {
    "_accessLevel": 2,
    "_id": "5fac57764f15fe8b6380c3e0",
    "_modelType": "tale",
    "authors": [],
    "category": "science",
    "config": {},
    "copyOfTale": null,
    "created": "2020-11-11T21:28:22.860000+00:00",
    "creatorId": "5f887c6a81d63263a60a657f",
    "dataSet": [],
    "dataSetCitation": [],
    "description": null,
    "folderId": "5fac57764f15fe8b6380c3e4",
    "format": 8,
    "icon": "https://raw.githubusercontent.com/whole-tale/jupyter-base/master/squarelogo-greytext-orangebody-greymoons.png",
    "iframe": true,
    "illustration": "https://raw.githubusercontent.com/whole-tale/dashboard/master/public/images/demo-graph2.jpg",
    "imageId": "5f8878fc263f3a13f9a12b82",
    "licenseSPDX": "CC-BY-4.0",
    "narrative": [],
    "narrativeId": "5f887d3b81d63263a60a65b4",
    "public": false,
    "publishInfo": [],
    "relatedIdentifiers": [
      {
        "identifier": "helloworld4",
        "relation": "IsSupplementTo"
      }
    ],
    "status": 0,
    "title": "helloworld4",
    "updated": "2020-11-11T21:28:22.860000+00:00",
    "workspaceId": "5fac57764f15fe8b6380c3e3"
  }
Xarthisius commented 3 years ago

@bodom0015 there should be a job created. If you go to girder UI select the dropdown under your username / My jobs.

There's gonna be a job called "Import a git repository as a Tale" with the following log:

RuntimeError: RuntimeError("Failed to import from git:\n Cmd('git') failed due to: exit code(128)\n  cmdline: git fetch -v origin\n  stderr: 'fatal: 'helloworld' does not appear to be a git repository\nfatal: Could not read from remote repository.\n\nPlease make sure you have the correct access rights\nand the repository exists.'")
[<FrameSummary file /girder/plugins/wholetale/server/tasks/import_git_repo.py, line 64 in run>]
bodom0015 commented 3 years ago

Ahhh, yep there I can see the jobs listed. Thank you!

Is it strange that these jobs are marked as Inactive? None of the three appear to have any Log Output, as the box is empty

Title Last update Type Status
Import a git repository as a Tale November 11, 2020 at 15:28:22 wholetale.import_git_repo Inactive
Import a git repository as a Tale November 11, 2020 at 12:55:45 wholetale.import_git_repo Inactive
Import a git repository as a Tale November 11, 2020 at 12:11:15 wholetale.import_git_repo Inactive
bodom0015 commented 3 years ago

Getting an error back on the PUT request:

RuntimeError: RuntimeError("Failed to import from git:\n Cmd('git') failed due to: exit code(128)\n  cmdline: git remote add origin https://github.com/nds-org/ndslabs\n  stderr: 'fatal: remote origin already exists.'")
[<FrameSummary file /girder/plugins/wholetale/server/tasks/import_git_repo.py, line 64 in run>]

Steps to reproduce:

  1. Create a Tale from https://github.com/nds-org/ndslabs-specs
  2. On the Run view, expand the dropdown and choose "Connect to Git Repository..."
  3. Enter https://github.com/nds-org/ndslabs and submit

The git remote add (as with PUT requests, in general) should probably be idempotent.

Xarthisius commented 3 years ago

Getting an error back on the PUT request:

RuntimeError: RuntimeError("Failed to import from git:\n Cmd('git') failed due to: exit code(128)\n  cmdline: git remote add origin https://github.com/nds-org/ndslabs\n  stderr: 'fatal: remote origin already exists.'")
[<FrameSummary file /girder/plugins/wholetale/server/tasks/import_git_repo.py, line 64 in run>]

Steps to reproduce:

  1. Create a Tale from https://github.com/nds-org/ndslabs-specs
  2. On the Run view, expand the dropdown and choose "Connect to Git Repository..."
  3. Enter https://github.com/nds-org/ndslabs and submit

The git remote add (as with PUT requests, in general) should probably be idempotent.

You can't have two .git repos in one directory... That's what your steps are trying to do. In order to test "Connect to Git Repository" I'd suggest changing 1. to create a regular Tale

bodom0015 commented 3 years ago

So do we need to detect if a Tale already has a gitRepo connected, and hide this dropdown option if so?

Is it possible (or is there even a reason to) "disconnect" a Tale from its git repo? I think I'm unclear on the expected user workflow for this case...

Xarthisius commented 3 years ago

So do we need to detect if a Tale already has a gitRepo connected, and hide this dropdown if so?

Is it possible (or is there even a reason to) "disconnect" a Tale from its git repo? I think I'm unclear on the expected user workflow for this case...

Nope, this feature is just a shortcut to do something in the UI and the working assumption is that it's being done once. You can "disconnect" by going to run > files > workspace and removing all files.

bodom0015 commented 3 years ago

I'm not sure how to remedy the user's confusion here...

I think we should probably hide or disable this dropdown option in this case (since the modal will never work if a repo has already been specified). I don't think that there is currently a way for me to detect from the client side that one has already been provided.

Is there a reason not to store the git url on the Tale itself? This would provide a way of knowing whether a git repo has already been linked.

Xarthisius commented 3 years ago

I'm not sure how to remedy the user's confusion here...

I think we should probably hide or disable this dropdown option (since the modal will never work if a repo has already been specified). I don't think that there is currently a way for me to detect from the client side that one has already been provided.

Is there a reason not to store the git url on the Tale itself? This would provide a way of knowing whether a git repo has already been linked.

No, cause clever user can use the Tale environment itself to convert workspace into .git repo. We are doing this for people who don't know how to do that. The only way of detecting if Tale is already based on .git would require doing a hard check on workspace content.

bodom0015 commented 3 years ago

If we're ok with offering this dropdown all the time, even in cases where it will never function properly, then this is an acceptable solution.

I do suspect that this workflow could actually make it even harder for the novice user to understand, especially if we don't hold their hand through it by disabling or hiding the unavailable or irrelevant options.

Xarthisius commented 3 years ago

I think the only reasonable thing we can do is present the error to the user, which something we're not doing now. wt_notification should fix that.

bodom0015 commented 3 years ago

This error does indeed come back in the notification-stream from the "Importing from Git" task.

The error message isn't easily understandable, so we may need to do some translating on that end.

Xarthisius commented 3 years ago

What are your thoughts on providing a way for the UI to know the tale is git-backed?

Wouldn't testing if workspace/.git exists suffice? However, the full answer to that question depends on what would you like to do with that info?

craig-willis commented 3 years ago

Disable/hide the "Connect to Git" menu. I don't think we should allow the user to try to connect at all if it's already a repo.

The UI should query the API for the existence of the directory as opposed to it being an attribute of the tale?