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

Add debug flag to build commands #7005

Open
humitos opened this issue Apr 30, 2020 · 26 comments
Open

Add debug flag to build commands #7005

humitos opened this issue Apr 30, 2020 · 26 comments
Assignees
Labels
Accepted Accepted issue on our roadmap Feature New feature

Comments

@humitos
Copy link
Member

humitos commented Apr 30, 2020

We have some commands shown in the output page that are useful only when debugging an issue with the build, but most of the time they are not and just add noise to the output page.

It would be good to implement a "Debug Mode" in the output page that you can click to show/hide these commands.

Reference: #6992 (comment)

@humitos humitos added Design Design or UX/UI related Accepted Accepted issue on our roadmap labels Apr 30, 2020
@humitos humitos added this to the Build output page milestone Apr 30, 2020
@humitos humitos added Needed: design decision A core team decision is required and removed Accepted Accepted issue on our roadmap labels Apr 30, 2020
@stsewd
Copy link
Member

stsewd commented May 7, 2020

I like the idea of toggling these commands in the build page 👍

@agjohnson agjohnson removed this from the Build output page milestone Jun 5, 2020
@agjohnson
Copy link
Contributor

I can start by mocking this in the new templates and JS that drives the view. It would be good to decide on how to do the modeling for a more permanent version. The most simple implementation of the JS changes would be that the build command knockout model would have a property is_debug that would be true for any command matching cat|mv|pip freeze.

This could eventually be done on the backend side at the point where the command is executed however.

@humitos
Copy link
Member Author

humitos commented Jun 15, 2020

@agjohnson checking for is_debug property from each BuildCommand makes sense. I think it will be easy to add something similar to record=False for those commands that we don't want to save into the db/show to the user.

We can call .run(..., debug=True) to automatically set BuildCommand.is_debug=True (or just name it debug) on those commands we want to mark as debug.

@agjohnson
Copy link
Contributor

Yeah, I like that plan! Any other decisions left here to discuss, or safe to bump this up to a work item?

@agjohnson agjohnson added this to the Private beta - community milestone Nov 23, 2021
@humitos
Copy link
Member Author

humitos commented Nov 23, 2021

The plan that we have sounds good to me!

@agjohnson agjohnson self-assigned this Dec 24, 2022
@agjohnson
Copy link
Contributor

agjohnson commented Jan 31, 2023

I was just thinking about this one more. What if instead we flipped this a little and added a configuration option:

debug: true

Or, alternatively, a debug flag on triggering builds functions, so there is a UI option for "Build version" and "Build version with debugging". This would be super handy in admin operations, though always having these commands available for inspection would be handy for us as well.

Debug mode on the build would execute all of the extra commands, like pip freeze, cat conf.py, a post_build tree -J for inspecting files, perhaps there's a bunch more here? It could also not sync files to production. In this case, the UI and modeling wouldn't need any changes, and these commands wouldn't be enabled by default. The overall effect is pretty similar to altering the UI and modeling though.

@humitos
Copy link
Member Author

humitos commented Feb 1, 2023

I think that I don't fully understand your proposal. So, nothing that I'm about to write may make sense 😄

At simple sight, it sounds pretty complex to me. It would add "another workflow to our build process" (do not upload docs --which could be pretty confusing for users) and also a new configuration in the YAML which I think it's not required.

With the addition of the proposal debug=True argument we won't ask the user to add any config in their file, and the debugging commands will be always available for them. They just need to click a UI element to turn it on/off; which looks pretty simple from a user perspective but also in the backend/frontend side.

@agjohnson
Copy link
Contributor

It would add "another workflow to our build process"

This doesn't feel this way to me, perhaps I'm not describing this well.

Right now, there is a build button on the project dashboard pages, and users can click that to start/restart a build. This effectively calls "trigger_build(project_pk)". I'm describing a duplicate button that would call "trigger_build(project_pk, debug=True)". To me, this feels like the same workflow we already enable.

The thing that I do like about unconditionally executing informational commands, and using a UI element to toggle them, is that the UX will be quite nice. Users (or us doing support) won't need to trigger a new build to get debug information. We're wasting storage for probably like 95% of builds, but this hasn't been an issue for a bit too.

The thing that I like about a debug flag on the build process is that users/support can trigger a build, and we could use the debug flag to end the process before syncing files to storage or updating the search index. This gives a way to test a build for a user without worrying about side effects.

There is room for both features too. A "Rebuild (debug mode)" button could skip updating hosted files and the search index, while a debug mode UI element could toggle debug level commands on/off.

@agjohnson
Copy link
Contributor

To push this forward, I'm going to preemptively add support for debug toggleing in the UI, and hack up detection for the commands that we do issue for informational purposes. The next step is still adding this to the modeling though, and removing my hacks.

If we're updating the modeling around build commands, I'd love to also get the build step included in the build command. This would allow for some nice UX around the command grouping and progress bar.

@agjohnson agjohnson removed the Design Design or UX/UI related label Feb 11, 2023
@agjohnson
Copy link
Contributor

agjohnson commented Feb 11, 2023

The templates now support build_command.is_debug, but this field can be renamed as needed. I'm going to hand this off for backend implementation and modeling.

Without debug enabled:

Image

With debug toggled on:

Image

@agjohnson agjohnson added Feature New feature Accepted Accepted issue on our roadmap and removed Needed: design decision A core team decision is required labels Feb 11, 2023
@humitos
Copy link
Member Author

humitos commented Feb 22, 2023

@agjohnson

Right now, there is a build button on the project dashboard pages, and users can click that to start/restart a build. This effectively calls "trigger_build(project_pk)". I'm describing a duplicate button that would call "trigger_build(project_pk, debug=True)". To me, this feels like the same workflow we already enable.

Gotcha. This explanation is a lot more clearer and I think I understand it correctly now. However, I'm not convinced this is a good path follow, just yet at least, and I'd prefer to move forward with the original idea (buildcommand.is_debug) for now and have a bigger conversation in the future about trigger_build(project_pk, debug=True).

@agjohnson
Copy link
Contributor

agjohnson commented Feb 22, 2023

Yeah, it could be implemented later if we want, and is still probably independent from debug mode altogether. This build mode would probably mostly for support, and for projects that don't have PR builds enabled -- so don't have as easy of an option for a throwaway build.

@ericholscher
Copy link
Member

Is this done? I've seen a debug mode :)

@agjohnson
Copy link
Contributor

There is UI to show debug elements in the UI, but no debug level commands. We talked a bit above about next steps to call this done, namely adding BuildCommand.is_debug so we can stop hacking these commands in at the template level

@agjohnson
Copy link
Contributor

agjohnson commented Jun 8, 2023

Also, I started on the templates by hacking in the configuration file with a mock command:

readthedocs-build --show-config

I sort of like this pattern and could see us using it for more debug output for the user. For this particular case, there wasn't a command that we could run to show the rendered config output, so I made one up.

This could be something to extend to other debug commands. For instance we talked about adding a debug command for showing the source config file:

cat .readthedocs.yaml

This isn't bad, though does require the user is familiar with what cat does and what the file .readthedocs.yaml is. Instead, we can make this more verbose and approachable with an option like:

readthedocs-build --show-config-source-file

Perhaps there are more commands we'd like to show in the build output in this way. It doesn't need an actual command underneath, though users might get cheeky and try to execute the "command" manually.

At very least though, I'd like to stop hacking this in at the dashboard JS and we'll eventually need a debug BuildCommand to show the rendered configuration file.

@humitos
Copy link
Member Author

humitos commented Jun 9, 2023

I would avoid showing "fantasy commands" because it will generate more confusion than other things. They are going to be harder to explain to users.

If we want to show the content of a file without using cat we can just use a description like "Configuration file used in this build" or any other verbose user friendly description that's easy to read.

However, there are things that could be better as UI elements instead, like "Show the configuration file used on this build" as a button/link or similar.

I'd start showing the real commands we want to use for debugging as a first step (witch is what we already have) and grow from there with any of the new proposals.

@agjohnson
Copy link
Contributor

I would avoid showing "fantasy commands" because it will generate more confusion than other things.

There are plenty of options we have here on improving display to make these more clear if we need -- more descriptive output, make it an actual command, etc. Or perhaps the rendered configuration is the wrong thing to be showing the user entirely and we don't need a fake command to show it.

But either way, we're already hacking in the output of multiple files using cat, and haven't had much (any?) negative feedback here. Plus this is debug information, so I'm not much worried about confusing users. But I do think there is something more user friendly than using cat.

Now, I will say that I don't particularly like the pattern of using cat to show these files, but I also don't want to make new or competing patterns for the build output. We should pick one pattern and stick with it.

If we want to show the content of a file without using cat we can just use a description

Would descriptions replace the command line? How would that mix with the rest of the lines?

I've avoided this option for a few reasons. Mixing commands and descriptions breaks the pseudo terminal feel of the page, which I think is a big loss. More importantly, I want consistency in the display output above anything. If we start to hide commands using descriptions, we should do this for all commands -- but I don't feel this is a good plan.

Replacing commands with descriptions masks the commands being executed and require users to search through the UI to find the command they want. There is more UI to add to this page that will help add some context to the build commands 1, but I'm not sure how I would work in descriptions for each command without complicating things.

Above all though, we're still just talking about debug commands, so I don't want the decisions here to drive the whole UI. I think we can keep our implementation lightweight here.

However, there are things that could be better as UI elements instead, like "Show the configuration file used on this build"

I did start here, but this is another place where I'd strive for consistency foremost. It's confusing to output some debug information as build commands (cat readthedocs.yaml, pip freeze) and some as separate UI. If we want this pattern, we should remove debug commands from the command listing.

However, inline debug information is still perfectly usable and can be found easily, so I don't necessarily want to move this information either.

I'd start showing the real commands we want to use for debugging as a first step (witch is what we already have)

Well, that's I think what we're describing above. Currently, the debug display is hacked in on the client side for now using regex searches, which I don't feel great about. The next step is to add BuildCommand.is_debug so we don't need to guess on the front end.

Footnotes

  1. There are additional UI elements that we need to add to this UI, such as labels for the build step of the command. I'm considering this as well when I consider UI bloat to this page.

@humitos
Copy link
Member Author

humitos commented Jun 9, 2023

But either way, we're already hacking in the output of multiple files using cat, and haven't had much (any?) negative feedback here. Plus this is debug information, so I'm not much worried about confusing users. But I do think there is something more user friendly than using cat.

This is a good point. Maybe there is no need to re-think everything here and continue with the pattern we currently have since it has been working fine for debugging purposes.

If we start to hide commands using descriptions, we should do this for all commands -- but I don't feel this is a good plan.

I understand and I tend to agree here. What about sticking with the pattern we currently have (running real commands to show debug information) but adding more context to those commands like help text that explain what they mean in a user-friendly manner. Maybe we can use the same pattern we are using in other places with a little icon that you can hover to get help text. I think I like this the most since gives us the best of the two worlds: keeps the pattern we are already using and gives confused users the help to understand these commands.

I'm taking about this UI element:

Screenshot_2023-06-09_10-05-48

@agjohnson
Copy link
Contributor

I was considering the same thing with tooltips, let's give that a try! I think that option could solve a few problems.

I still would want to continue on with debug commands though, but would want to consider better options for getting the user debug information like the source/final configuration file, pip freeze, etc.

I have a few updates to this UI that I want to add, and can at least mock in copy to test the UI changes. When I can prioritize this work, I can start with some mock ups, and it might really help to pair on this and test out some of the options.

@agjohnson agjohnson changed the title Add debug mode to the build output page Add debug flag to build commands Jun 9, 2023
@agjohnson
Copy link
Contributor

Another option for cat .readthedocs.yaml could be not just outputting the file, but being descriptive about what we're actually trying to do:

% readthedocs-build --run-build-checks
✔️ Configuration found at `.readthedocs.yaml`
✔️ Configuration successfully parsed
✔️ Another passing check

Again, this might be a mocked BuildCommand, using data we have in the build already instead of trying to do this from the build environment.

We may want both commands. Or maybe this command above isn't even a debug command but a command that runs every build, so it's more prominent every build.

@humitos
Copy link
Member Author

humitos commented Jun 14, 2023

I prefer to avoid fantasy commands if possible. I'd prefer to use the new build notifications to show warnings / tips or similar for those checks we want to do instead.

@agjohnson
Copy link
Contributor

agjohnson commented Jun 14, 2023

This would be neither a warning or tip though, and would be in competition with the real estate for actual warnings/errors on every build.

It also does not have to be a fantasy command, but I also don't see what the anticipated issue would be with using a descriptive command. Is there a particular scenario you are worried about here?

I feel users will get more benefit out of descriptive commands (over cat ...) than they would harm out of trying to execute an internal command, which is unlikely to happen either way.

Either way, I want to avoid drawing the user in multiple direction in the UI. Putting some debug information in the command list and some elsewhere in the UI will be confusing.

@agjohnson
Copy link
Contributor

I'm pushing off the discussion not related to adding a BuildCommand.is_debug field to readthedocs/ext-theme#171. None of that needs to interfere with this issue, and the next step there is experimenting with the display more.

I would like to get this particular issue in so we can show/hide debug commands more consistently. It seems like we still want this, correct?

@humitos
Copy link
Member Author

humitos commented Jun 15, 2023

I would like to get this particular issue in so we can show/hide debug commands more consistently. It seems like we still want this, correct?

Yes. This is the main goal of this issue/work and I understand we all want that 👍🏼 .

We can continue the conversation about "what are those commands?" in #readthedocs/ext-theme#171 as you mentioned.

@agjohnson
Copy link
Contributor

Coming back to this, I had some more thoughts here. This applies to what we are trying to do in #11097 too.

The big point for me against relying on cat is that it is the bare minimum we can add to the UX here, and there really feels like much better UX options.

For example, cat .readthedocs.yaml both hides what we're really doing in the build process at this time, and doesn't give the user the information we're trying to communicate.

In the build process at this point, we are:

  • Checking the configuration file exists
  • Validating the configuration file

The cat command:

  • Shows the contents of the file

That is, cat doesn't communicate when the file doesn't exist, and it doesn't show validation is happening.

I feel #11097 could be an improvement if it shows when the file is missing:

% cat .readthedocs.yaml
Error: file does not exist

I would take this one step further and say this shouldn't be cat at all, and instead should be something closer to yamale (yamale is an example and not important here and obviously there are barriers here like maintaining an actual yamale schema):

% yamale .readthedocs.yaml
Error: file does not exist
% yamale .readthedocs.yaml
Error line 20: build.os is missing

But if this were the case, I would say this issue is not super valid, as I would want to show these commands to users, not hide them.

Otherwise, a debug flag feels important to me still, as cat is cryptic and informational in a minimal way.

@agjohnson agjohnson removed this from the Template private beta - community milestone Feb 29, 2024
@humitos
Copy link
Member Author

humitos commented Mar 1, 2024

Yes, cat is not a replacement for the better UX we are talking about here. It's just the minimal and easier way to implement a step that gives us an improvement. However, it doesn't fix the issue at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Feature New feature
Projects
Status: Planned
Development

No branches or pull requests

4 participants