Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible option to view inputs and fix a bug #120

Merged
merged 5 commits into from
Aug 26, 2024
Merged

Possible option to view inputs and fix a bug #120

merged 5 commits into from
Aug 26, 2024

Conversation

sckott
Copy link
Collaborator

@sckott sckott commented Aug 9, 2024

fixes #85

This approach uses listviewer pkg suggested by Dan.

It's just unwieldy to view a nested structure like JSON or an R list in a small window within a web page (e.g, if we tried to use the same window in the tracking tab where the current inputs table lives), so I went with a whole new page for viewing the inputs structure.

Run locally, and/or have a look at the below gif

stuff3

Thoughts?

Alternatives:

  • just a download button to download the json
  • some other solution, there doesn't appear to be obvious alternatives to listviewer
  • when we move to proof 2 in JS land we'll have other options but that's down the road

- new viewer tab to view inputs only with reactjson
- replace inputs table in tracking tab with button to link to viewer tab
- button in viewer tab to link back to tracking tab
@sckott
Copy link
Collaborator Author

sckott commented Aug 9, 2024

also @sitapriyamoorthi what do you think

@sitapriyamoorthi
Copy link
Collaborator

@sckott loving the gif :) will test locally as well.

Here are my thoughts based on what I am seeing:

  1. I think this solves the problem of the table making a fuss. Meaning a user can actually see their input JSON. So yeay!!!
  2. However the question is .. does this address the users need. Based on @tefirman and @vortexing use cases looks like that they use it primarily when they are tracking jobs. Would it perhaps be more fitting to have it be a button in the track jobs page or maybe even troubleshoot page? Rather than a separate page as it appears currently.
  3. The next thing is what do the users want to see? Do they want to see just a list of all their inputs or do they want to see inputs by task? Do they want to know the full paths? My gut says in an ideal world (and if and only if it isnt too complicated) as a user i would like to see my what inputs each of my task is using and what exact file (full path) is it using. So in situations where my workflows are failing on specified tasks I can check if the right inputs are being fed or not.
  4. The viewer is essentially allowing you to see your input JSON. Perhaps this is also a good opportunity to parse the WDL as well so that the user can also view the WDL. And the troubleshooting could be facilitated on the app itself.

Scenario: Oops I have a failed workflow! Let me look at my JSON and my WDL right here on the app rather than having to go to my saved versions of them in some folder I may or may not remember 😸

Happy to chat more if needed.

@sckott
Copy link
Collaborator Author

sckott commented Aug 12, 2024

Thanks @sitapriyamoorthi !

  1. Can you clarify this - I don't understand, a button that does what? 👉🏽 "Would it perhaps be more fitting to have it be a button in the track jobs page or maybe even troubleshoot page?"
  2. Okay. What kind of structure in the app would that entail? A table? Something else?
  3. Good idea. Seems very doable. Let's see if others like that idea

@sitapriyamoorthi
Copy link
Collaborator

  1. I meant to say could we have a way to display the JSON or whatever format you decide for the inputs to be displayed within the track jobs page itself rather than a seperate tab?
  2. I think a table or a list would either be find. just something that is more easy to parse through
  3. Cheers

@sckott
Copy link
Collaborator Author

sckott commented Aug 14, 2024

  1. The only way I think would make sense to include in the track jobs page is if we used a card from bslib with the option to allow the user to go full screen (see https://rstudio.github.io/bslib/reference/card.html#arg-full-screen). But that is not an option unless we incorporate this issue Replace shinydashboard with bslib #49 Otherwise, we could just have a button to download the output IF we don't want a separate page for viewing the json
  2. Okay, would need to see what this looks like. Maybe we can chat about this soon

@sckott
Copy link
Collaborator Author

sckott commented Aug 15, 2024

@tefirman any thoughts on this?

@tefirman
Copy link
Collaborator

First off, LOVE the json viewing functionality, I think that's exactly what users will need for this purpose, thanks for implementing that, @sckott !

Second, the separate tab nature of this tool actually produced a lightbulb for me while we were at ShinyConf:

  • The more I look at it, I think the "Track Jobs" tab has way too much going on. Philosophically, I like that every other tab is relatively minimal, i.e. I only have to scroll once (if that) to see all of the information on that tab.
  • To that end, I'm wondering if we can split the "Track Jobs" tab into two separate tabs. Specifically, can we move all job-specific tables (i.e. everything from "Workflow Specific Job Information" and below) onto that new tab and just call it "Job Details"?
  • With this change, "Track Jobs" would become more of a macroscopic view of your submissions and you can toggle back and forth to the "Job Details" tab to view specifics for each submission.

Please feel free to tell me this is insane, just wanted to at least throw it out there, felt like it might clean up the interface a bit...

To address @sitapriyamoorthi 's points:

  • 2 & 3: As long as I can easily toggle back and forth between the tabs, I think this is exactly what I would want as a user in terms of a quick look up of the inputs/options/WDL. I don't need all of the info to be on that one "Track Jobs" table, I just need it to be easily and quickly accessible for each job. To Sita's point though, we could maybe make that toggling easier by adding a second button in the "Track Jobs" table that's analagous to the "Back to Track Jobs Tab" button but in reverse? Maybe "Get Job Details" or something? Maybe it could replace the "copyId" column?
  • 4: Totally agree, if this viewer is easily transferable to apply to WDL scripts, I think that's a perfect addition.

@sckott
Copy link
Collaborator Author

sckott commented Aug 16, 2024

Thanks for having a look @tefirman !

It'd be easy enough to move the workflow specific job information to a new page. But we should see what folks think. We should open a new issue for this though. (#123)

Sounds like people like the addition of viewing the WDL too. See new issue #122

we could maybe make that toggling easier by adding a second button in the "Track Jobs" table that's analagous to the "Back to Track Jobs Tab" button but in reverse? Maybe "Get Job Details" or something? Maybe it could replace the "copyId" column?

I'll see about adding that

@sckott
Copy link
Collaborator Author

sckott commented Aug 19, 2024

#125 is now related to this PR ... kinda waiting on that PR before proceeding here

@tefirman
Copy link
Collaborator

@sckott -- Started looking into this PR again and noticed it is set to merge to main. Can you switch that to dev?

Also wondering if it's worth switching the default branch to dev in all PROOF-based repos so we avoid that moving forward..

@tefirman tefirman changed the base branch from main to dev August 26, 2024 18:03
@tefirman
Copy link
Collaborator

@sckott -- nevermind, just realized I can do that myself, I thought it could only be done by the author, my bad... hahaha.

@sckott
Copy link
Collaborator Author

sckott commented Aug 26, 2024

@tefirman thx for switching it to dev

switching the default branch to dev in all PROOF-based repos so we avoid that moving forward

not a bad idea. I have seen that in other repos on GitHub. I don't know what the downsides are, if any. And we'd need to check if there's any consequences of the default branch being changed for the infra side @dtenenba

@tefirman
Copy link
Collaborator

As mentioned in #125, let's merge both this into dev and we can iterate from there. Both tools do what we want them to do, we just need a bit of testing/refinement, which is much easier to do in dev. Might be cool to combine them into one tab, e.g. "Job Details", but again, we can iterate on this to figure out what works best.

Thanks @dtenenba and @sckott for your work on this!!

@tefirman tefirman merged commit 8d95a27 into dev Aug 26, 2024
1 check passed
@dtenenba
Copy link
Contributor

I can't think of any downsides. The CI/CD stuff is based explicitly on branch names, not on which branch is the default.

@sckott
Copy link
Collaborator Author

sckott commented Aug 27, 2024

thanks @dtenenba just thought I'd check

@sckott
Copy link
Collaborator Author

sckott commented Aug 27, 2024

@tefirman so if you want to change default to dev I think it's all good

@sckott sckott deleted the inputs-viewer branch August 27, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Differing numbers of rows error
4 participants