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

Jump to bottom

Cleaned up logging a little #3836

Merged
merged 2 commits into from
Sep 25, 2020
Merged

Cleaned up logging a little #3836

merged 2 commits into from
Sep 25, 2020

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Sep 23, 2020

Follow up to #3791 focused on comments from @oliver-sanders reproduced here:

The log messages contain a bit of duplicated info and could do with being tidied up a bit:

DEBUG - ['bash', '-c', 'echo
	localhost']
DEBUG - [remote-host-select cmd] bash -c
	'echo localhost'
	[remote-host-select ret_code] 0
	[remote-host-select out] localhost
DEBUG - for task foo.1: platform =
	$(echo localhost) evaluated as localhost

One message should suffice for this case, note the second log message comes from the subprocpool and contains all the required info.

DEBUG - Traceback (most recent call
	last):
	  File "/Users/oliver/cylc-flow/cylc/flow/task_job_mgr.py", line 866, in
	_prep_submit_task_job
	    platform = get_platform(rtconfig)
	  File "/Users/oliver/cylc-flow/cylc/flow/platforms.py", line 82, in get_platform
	    output = platform_from_name(task_conf['platform'])
	  File "/Users/oliver/cylc-flow/cylc/flow/platforms.py", line 184, in
	platform_from_name
	    f"No matching platform \"{platform_name}\" found")
	cylc.flow.exceptions.PlatformLookupError: No matching platform "elephant" found

ERROR - No matching platform "elephant" found
ERROR - [jobs-submit cmd] (platform not defined)
	[jobs-submit ret_code] 1
	[jobs-submit err] No matching platform "elephant" found

One message should suffice for this case, the traceback isn't helpful here as we already know where the exception was raised from.

Treating each message in order:

  1. Seems to be heavily baked into subprocpool.py: I'm a bit reluctant to remove this, since I haven't changed it, and I fear that other things may require it.
  2. Got rid of.
  3. Kept.
  4. Got rid of.
  5. Got rid of.
  6. Kept.

@wxtim wxtim added could be better Not exactly a bug, but not ideal. platforms-follow-up labels Sep 23, 2020
@wxtim wxtim added this to the cylc-8.0a3 milestone Sep 23, 2020
@wxtim wxtim self-assigned this Sep 23, 2020
hjoliver
hjoliver approved these changes Sep 24, 2020
View reviewed changes
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Seems reasonable, thanks @wxtim 👍

@oliver-sanders
Copy link
Member

Yep, we should keep the subprocpool logging, it is very helpful.

Can remove (3) as well since it doesn't give us any information that (2) doesn't, note (2) will also give us stderr if it occurs which is very helpful.

@hjoliver
Copy link
Member

Yep, we should keep the subprocpool logging, it is very helpful.

Can remove (3) as well since it doesn't give us any information that (2) doesn't, note (2) will also give us stderr if it occurs which is very helpful.

Are you suggesting restoring (2) and removing (3) @oliver-sanders ? (@wxtim did the opposite of that).

@wxtim
Copy link
Member Author

wxtim commented Sep 25, 2020 •
edited
Loading

Are you suggesting restoring (2) and removing (3) @oliver-sanders ? (@wxtim did the opposite of that).

Happy to do whatever, but would prefer agreement first...

@oliver-sanders
Copy link
Member

Aah, I thought (2) had come from the subprocpool, but it was (1).

@oliver-sanders oliver-sanders merged commit bd29395 into cylc:master Sep 25, 2020
wxtim added a commit to wxtim/cylc that referenced this pull request Sep 25, 2020
@wxtim
Merge branch 'master' into platforms.eval_platform_cmd.no_backticks
8b367a3 
* master: (36 commits)
  Cleaned up logging a little (cylc#3836)
  Platform from platform group basic unit test (cylc#3832)
  added reqd test_header
  tid platform tests
  scan: improve colour output
  spelling error fix
  scan: fix hier suites
  test: old host/batch system configs don't conflict with dummy-local mode
  Fix task message validation for multiline messages
  Fix spelling error.
  removed cylc7 data structure
  removed unused endpoints
  Commands migrated to GraphQL endpoint
  cli: rename spawn -> set-outputs
  Update changelog
  Remove test_unicode_rules
  Do not allow any colons in task outputs
  Fix command help.
  Correct typo.
  Upgraded new test.
  ...
@hjoliver hjoliver modified the milestones: cylc-8.0a3, cylc-8.0b0 Feb 25, 2021
@wxtim wxtim deleted the platforms.eval_platform_cmd.prune_logging branch March 22, 2022 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hjoliver hjoliver hjoliver approved these changes

@oliver-sanders oliver-sanders oliver-sanders approved these changes

Assignees

@wxtim wxtim

Labels
could be better Not exactly a bug, but not ideal.
Projects
None yet
Milestone
cylc-8.0b0
Development

Successfully merging this pull request may close these issues.
3 participants
@wxtim @oliver-sanders @hjoliver