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.

Sign up for GitHub

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

Fix small issues on Joblist #3767

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Fix small issues on Joblist #3767

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bcbb3a4
Wrap results in a modal
chrisvanrun Dec 24, 2024
4722b78
Move error to modal in status column
chrisvanrun Dec 27, 2024
bcbb087
Fix curly linter errors
chrisvanrun Dec 27, 2024
c8dea63
Isolate curly linter error
chrisvanrun Dec 27, 2024
c1005b0
Make warnings and errors more similar in visual appearance
chrisvanrun Jan 10, 2025
bf8a281
Minor aria fixes
chrisvanrun Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 42 additions & 53 deletions app/grandchallenge/algorithms/templates/algorithms/job_list_row.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,71 +46,60 @@
<split></split>

{{ object.comment }}
<split></split>

<div class="child-p-mb-0 text-left">
{{ object.rendered_result_text }}
</div>

<split></split>
<ul class="list-unstyled mb-0 text-left">
{% if object.error_message %}

<div class="modal"
id="errorInfoModal-{{ object.pk }}"
tabindex="-1"
role="dialog"
aria-labelledby="statusfoModalTitle"
aria-hidden="true"
>
<div class="modal-dialog modal-dialog-centered modal-lg" role="document">
<div class="modal-content">
<div class="modal-header">
<h5 class="modal-title">
Error Message
</h5>
<button type="button" class="close" data-dismiss="modal"
aria-label="Close">
<span aria-hidden="true">&times;</span>
</button>
</div>
<div class="text-left modal-body">
{{ object.error_message }}
</div>
<div class="modal-footer">
<a class="btn btn-primary" href="{{ object.get_absolute_url }}">
<i class="fa fa-info-circle mr-1"></i>View Result Details
</a>
</div>
{% if object.rendered_result_text %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we show a link to the result even if the rendered_result_text is empty? For algorithms that do not implement the legacy "results-json-file" this is just an empty string so the results column is empty:

Screenshot 2025-01-14 at 15 06 50

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we hide the Result column if there is no rendered text? I think I had that implemented at one point but it got reversed. Maybe because it wasn't implemented correctly at the time.

Copy link
Member

@jmsmkn jmsmkn Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That had problems as some jobs had rendered text, some didn't, so the column would hide and unhide when crossing pages, and sometimes it would be invisible when it should have been visible (#3524). A query over all jobs to determine its visibility would have needed to be done which means does any job have the right output and is the rendered text returning something for that job. Too expensive for algorithms with lots of jobs.

I think the best way forward here is to link to the detail page again if it is empty for a particular job. We can have another pitch/issue to decide whether to remove the rendered results text now as this is legacy stuff and users should have moved to specific outputs. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of having the badge always show up: for now. I think an argument can be made that we should remove this small part of the feature since we don't actively suggest people to use it anymore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, could you add the badge and stderr filtering?

<div class="modal"
id="renderedResultModal-{{ object.pk }}"
tabindex="-1"
role="dialog"
aria-labelledby="statusfoModalTitle"
aria-hidden="true"
>
<div class="modal-dialog modal-dialog-centered modal-lg" role="document">
<div class="modal-content">
<div class="modal-header">
<h5 class="modal-title">
Result
</h5>
<button type="button" class="close" data-dismiss="modal"
aria-label="Close">
<span aria-hidden="true">&times;</span>
</button>
</div>
<div class="text-left modal-body">
{{ object.rendered_result_text }}
</div>
<div class="modal-footer">
<a class="btn btn-primary" href="{{ object.get_absolute_url }}">
<i class="fa fa-info-circle mr-1"></i>View Result Details
</a>
</div>
</div>
</div>
</div>

<li>
<a href="#errorInfoModal-{{ object.pk }}"
data-toggle="modal"
data-target="#errorInfoModal-{{ object.pk }}"
class="text-decoration-none"
>
<div class="badge badge-danger">
<i class="fa fa-fw fa-eye"></i>
Error
</div>
<div class="badge badge-light">
{{ object.error_message | truncatechars:16 }}
</div>
</a>
</li>
{% endif %}

<a class="badge badge-primary"
href="#renderedResultModal-{{ object.pk }}"
data-toggle="modal"
data-target="#renderedResultModal-{{ object.pk }}"
class="text-decoration-none"
>
<i class="fa fa-fw fa-eye"></i>
Result
</a>
{% endif %}

<split></split>

<ul class="list-unstyled mb-0 text-left">
{% for civ in object.outputs.all|sort_civs %}
<li>
{% include "components/partials/civ.html" with object=civ display_inline=True only %}
</li>
{% empty %}
{% if not object.error_message %}
<span class="text-muted">Empty</span>
{% endif %}
<span class="text-muted">Empty</span>
{% endfor %}
</ul>
<split></split>
Expand Down
97 changes: 90 additions & 7 deletions app/grandchallenge/algorithms/templates/algorithms/job_status_detail.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,90 @@
<span class="badge badge-{{ object.status_context }}" {% if not object.finished %}hx-get="{{ object.status_url }}" hx-trigger="load delay:30s" hx-swap="outerHTML"{% endif %}>
{% if object.animate %}
<span class="spinner-border spinner-border-sm" role="status" aria-hidden="true"></span>
{% endif %}
{{ object.get_status_display }}{% if object.status == object.SUCCESS and object.stderr %},
with warnings{% endif %}
</span>
{% if object.status == object.SUCCESS and object.stderr %}
<div>
<div class="modal"
id="warningInfoModal-{{ object.pk }}"
tabindex="-1"
role="dialog"
aria-hidden="true"
>
<div class="modal-dialog modal-dialog-centered modal-lg" role="document">
<div class="modal-content">
<div class="modal-header">
<h5 class="modal-title">
Warnings on stderr
</h5>
<button type="button" class="close" data-dismiss="modal"
aria-label="Close">
<span aria-hidden="true">&times;</span>
</button>
</div>
<div class="text-left modal-body">
{{ object.stderr }}
</div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will dump the entirety of stderr, I think this needs to be passed through user_error like in the components application. Or maybe that work should be done when the job completes so that object.error_message is populated and these two conditional blocks can be unified?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. A stack trace can generally be summarized using the last line as that one is already quite informative. Warnings are far less obvious and vary greatly between frameworks: I guess that is why we rely on the presence of any stderr. Then again, dumping the stderr whole is overkill.

Would it make sense to have an object.warning_message that gets populated when stderr is set? We could truncate things to the first, say, 10 lines (without timestamps)? That way things are not as heavy when viewing many jobs in a list at the same time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the existing contract we offer to the user already though - we will pass on the last non-empty line of stderr. It is then up to the algorithm developer to do their best with that. I think we should do that filtering using the existing user_error filter now as changing object.error_message may have unintended side effects and the whole of stderr often contains gigantic progress bars.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the whole of stderr often contains gigantic progress bars.

I noticed that in spot checks. Quite annoying.

<div class="modal-footer">
<a class="btn btn-primary" href="{{ object.get_absolute_url }}">
<i class="fa fa-info-circle mr-1"></i>View Result Details
</a>
</div>
</div>
</div>
</div>

<a href="#warningInfoModal-{{ object.pk }}"
data-toggle="modal"
data-target="#warningInfoModal-{{ object.pk }}"
class="badge badge-{{ object.status_context }}"
title="Click to see the warnings"
>
<i class="fa fa-fw fa-eye" aria-hidden="true"></i>
{{ object.get_status_display }}, with warnings
</a>
</div>
{% elif object.status == object.FAILURE and object.error_message %}
<div>
<div class="modal"
id="errorInfoModal-{{ object.pk }}"
tabindex="-1"
role="dialog"
aria-hidden="true"
>
<div class="modal-dialog modal-dialog-centered modal-lg" role="document">
<div class="modal-content">
<div class="modal-header">
<h5 class="modal-title">
Error Message
</h5>
<button type="button" class="close" data-dismiss="modal"
aria-label="Close">
<span aria-hidden="true">&times;</span>
</button>
</div>
<div class="text-left modal-body">
{{ object.error_message }}
</div>
<div class="modal-footer">
<a class="btn btn-primary" href="{{ object.get_absolute_url }}">
<i class="fa fa-info-circle mr-1"></i>View Result Details
</a>
</div>
</div>
</div>
</div>

<a href="#errorInfoModal-{{ object.pk }}"
data-toggle="modal"
data-target="#errorInfoModal-{{ object.pk }}"
class="badge badge-{{ object.status_context }}"
title="Click to see the error message"
>
<i class="fa fa-fw fa-eye" aria-hidden="true"></i>
{{ object.get_status_display }}, with errors
</a>
</div>
{% else %}
<span class="badge badge-{{ object.status_context }}">
{% if object.animate %}
<span class="spinner-border spinner-border-sm" role="status" aria-hidden="true"></span>
{% endif %}
{{ object.get_status_display }}
</span>
{% endif %}
Loading