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

Dump parallel failure to log unless "debug" is "true" #1645

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oerc0122
Copy link
Collaborator

@oerc0122 oerc0122 commented Apr 23, 2024

Fixes #1609

@oerc0122 oerc0122 added the Parallel Task needing parallelisation label Apr 23, 2024
@oerc0122 oerc0122 self-assigned this Apr 23, 2024
@oerc0122 oerc0122 marked this pull request as ready for review April 23, 2024 14:47
Copy link
Collaborator

@cmarooney-stfc cmarooney-stfc left a comment

Choose a reason for hiding this comment

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

needs a test

Comment on lines 137 to 142
function debug_print(varargin)
if debug
fprintf(varargin{:});
else
fprintf(fh, varargin{:});
end
end
Copy link
Member

@abuts abuts Apr 23, 2024

Choose a reason for hiding this comment

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

completely agree with need for test. Absolutely not-obvious that fh is global variable and it propagated correctly.

The idea itself looks reasonable.

Copy link
Member

@abuts abuts left a comment

Choose a reason for hiding this comment

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

needs test

@oerc0122 oerc0122 force-pushed the silence-par-errors branch from 66f81bf to 02ce952 Compare May 7, 2024 15:03
@oerc0122 oerc0122 changed the title [WIP] Dump parallel failure to log unless "debug" is "true" Dump parallel failure to log unless "debug" is "true" May 7, 2024
@oerc0122 oerc0122 requested review from abuts and cmarooney-stfc May 7, 2024 15:04
Comment on lines +43 to +47
MExceptions_outputs(i) = true;
debug_print('Task %d failed. Error %s; Message %s\n', ...
i, outputs{i}.identifier, outputs{i}.message);

Copy link
Member

@abuts abuts May 13, 2024

Choose a reason for hiding this comment

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

I was playing with various failing jobs during the week and would argue that this should be reconsidered.
When job fails, it prints short list of messages stating worker failed (one row)
Then it prints detailed information.
The detailed information should go into log, that's correct.
But short info about job failure should remain. Then it should be one string
see file .... for details of the issue.

I do not want to investigate this issue too deeply, but have a feeling that modified changes are too brief. The suggested solution would provide much better user experience.

Copy link
Member

@abuts abuts left a comment

Choose a reason for hiding this comment

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

suggestion done below

@oerc0122 oerc0122 force-pushed the silence-par-errors branch 4 times, most recently from 7efef8c to bdcd1e5 Compare May 15, 2024 12:55
@oerc0122 oerc0122 force-pushed the silence-par-errors branch from bdcd1e5 to a5a8f13 Compare January 8, 2025 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Parallel Task needing parallelisation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User Request: Condense parallel warnings
3 participants