Problem: replacement of "suite" with "workflow" breaks review. #4233

wxtim · 2021-05-27T11:27:36Z

This is a small change with no associated Issue.

Cylc Review Broken by name changes in the cylc flow database of "suite_params" → "workflow_params"

Testing is manual, I'm afraid.
Suggest @MetRonnie to review & @dpmatthews to test

See comments in code for explanation.

Requirements check-list

I have read CONTRIBUTING.md and added my name as a Code Contributor.
Contains logically grouped changes (else tidy your branch by rebase).
Appropriate change log entry included.
No documentation update required.

Fix: Re-write method `is_cylc8`.

wxtim · 2021-05-27T12:58:16Z

lib/cylc/review_dao.py

        suite_info = self._db_exec(
-            user_name, suite_name, 'SELECT * FROM suite_params', []


This would fail for workflows run with Cylc 8 after the bulk renaming of "suite" → "workflow"

wxtim · 2021-05-27T12:59:54Z

lib/cylc/review_dao.py

-
-        if cylc_version and cylc_version[0] == '8':
+        suite_info = [i[0] for i in suite_info]
+        if 'workflow_params' in suite_info:


This may cause problems with very early versions of Cylc 8 (before the renaming of "suite_params" → "workflow_param"), but should be OK from now.

MetRonnie · 2021-05-27T15:19:41Z

I think this should be on the 7.8.x milestone?

MetRonnie

LGTM. I'm not too familiar with cylc review, how should I test?

hjoliver · 2021-05-27T20:50:37Z

I think this should be on the 7.8.x milestone?

Good question. It's on the 7.8.x branch, but needed for compatibility for the next 8 release. But we'll need a 7.8 release to get it published. ~~So, yes, I'll change the milestone.~~ [Actually, it may be better to leave it on the 8 milestone, so it will appear as a reminder to check that the 7 release has been done...]. Ideally it would be on both milestones!

wxtim · 2021-05-27T22:05:53Z

LGTM. I'm not too familiar with cylc review, how should I test?

I've been working on the basis of starting long running workflows on both Cylc 7 and Cylc 8. Then on then Cylc 7 branch run cylc review start. Then open http://localhost:8080, and fossick around until you find something that I've not fixed.

There are, according to talk at our team meeting, tests. I have no idea if these are functional on master, let alone this branch.

wxtim · 2021-05-27T22:12:39Z

I think this should be on the 7.8.x milestone?

It's a touch odd for the reason @hjoliver outlines: I plonked for Cylc 8 because Cylc Review works perfectly well for Cylc 7.

dpmatthews · 2021-05-28T16:01:17Z

The only problem I can see is that this doesn't appear to be displaying the log files in log/workflow (was log/suite).

MetRonnie · 2021-05-28T16:49:40Z

I have tested as described. cylc review seems to be working fine, apart from lack of log files.

(My cylc 8 workflow didn't seem to run properly, but I suspect that's just because I had to set CYLC_HOME to a different path than normal to get a cylc 7 suite to run)

Update: I got a 500 internal server error with traceback OperationalError: no such table: suite_params, but I think this happened after I switched back to the 7.8.x branch from this PR branch.

wxtim · 2021-05-29T07:59:08Z

I've just had a look at the testing: It's broken on branch:7.8.x. I'll take a quick look, but I don't want to spend too much time fixing it.
I'll fix the issue with the logfiles and get back to you. Thanks for the reviews.

wxtim · 2021-05-31T12:06:35Z

The only problem I can see is that this doesn't appear to be displaying the log files in log/workflow (was log/suite).

I think that I've fixed this now.

MetRonnie

I can now access the latest log (the symlink one) of a Cylc 8 workflow.

However, when I try to access a specific log file (one with a datetime in the filename), I get a 404 page with this traceback, both for Cylc 7 & 8 workflows:

Traceback (most recent call last):
  File "~/github/cylc-7/lib/cherrypy/_cprequest.py", line 679, in respond
    response.body = self.handler()
  File "~/github/cylc-7/lib/cherrypy/lib/encoding.py", line 230, in __call__
    self.body = self.oldhandler(*args, **kwargs)
  File "~/github/cylc-7/lib/cherrypy/_cpdispatch.py", line 60, in __call__
    return self.callable(*self.args, **self.kwargs)
  File "~/github/cylc-7/lib/cylc/review.py", line 666, in view
    user, suite, path, path_in_tar=path_in_tar, mode=mode)
  File "~/github/cylc-7/lib/cylc/review.py", line 505, in get_file
    f_name = self._get_user_suite_dir(user, suite, path)
  File "~/github/cylc-7/lib/cylc/review.py", line 930, in _get_user_suite_dir
    return self._check_dir_access(path)
  File "~/github/cylc-7/lib/cylc/review.py", line 789, in _check_dir_access
    raise cherrypy.HTTPError(404)
HTTPError: (404, None)

Seeing as it happens for both 7/8, I guess this is a separate issue

wxtim · 2021-06-01T10:54:37Z

However, when I try to access a specific log file (one with a datetime in the filename), I get a 404 page with this traceback, both for Cylc 7 & 8 workflows:

I think I see the problem (the file name has a timezone appended?). I'll see if I can fix it in this issue before we merge. Might as well.

oliver-sanders · 2021-06-04T12:43:11Z

Waiting on resolution of the last comment.

wxtim · 2021-06-11T09:38:26Z

@MetRonnie & @oliver-sanders I've double checked that the issue with failing to show files with a timezone in the name is not peculiar to this branch and have created issue #4260 so that this PR can be closed.

oliver-sanders · 2021-06-11T09:48:16Z

(I don't think we need four reviews)

wxtim · 2021-06-11T09:50:35Z

(I don't think we need four reviews)

I don't really either. I was wanting Dave to keep an eye, but I guess he can check 7.8.x periodically and raise issues.

I shall merge if I don't hear any objections soon.

oliver-sanders · 2021-06-11T09:51:12Z

@MetRonnie happy your comment above has been captured? If so please merge.

wxtim self-assigned this May 27, 2021

wxtim added the bug Something is wrong :( label May 27, 2021

wxtim modified the milestones: cylc-8.0.0, cylc-8.0b2 May 27, 2021

wxtim requested review from dpmatthews and MetRonnie May 27, 2021 12:52

Problem: replacement of "suite" with "workflow" breaks review.

5cf9ee9

Fix: Re-write method `is_cylc8`.

wxtim force-pushed the cylc-review-fixes.tasklist_broken_by_workflow branch from cedb75c to 5cf9ee9 Compare May 27, 2021 12:53

wxtim commented May 27, 2021

View reviewed changes

MetRonnie approved these changes May 27, 2021

View reviewed changes

wxtim requested review from hjoliver and removed request for dpmatthews May 27, 2021 22:13

Cylc review now checks for log/workflow/log* and file-installation.log*

43de864

wxtim requested review from MetRonnie and dpmatthews May 31, 2021 12:09

wxtim mentioned this pull request May 31, 2021

Cylc review fixes.skip links to source #4236

Merged

5 tasks

MetRonnie approved these changes Jun 1, 2021

View reviewed changes

oliver-sanders approved these changes Jun 4, 2021

View reviewed changes

oliver-sanders removed request for dpmatthews and hjoliver June 11, 2021 09:47

MetRonnie merged commit 8c24a7e into cylc:7.8.x Jun 11, 2021

wxtim deleted the cylc-review-fixes.tasklist_broken_by_workflow branch June 11, 2021 11:21

MetRonnie modified the milestones: cylc-8.0b2, cylc-7.8.x Jul 22, 2021

hjoliver modified the milestones: cylc-7.8.x, cylc-7.8.9 and 7.9.4 Sep 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Problem: replacement of "suite" with "workflow" breaks review. #4233

Problem: replacement of "suite" with "workflow" breaks review. #4233

wxtim commented May 27, 2021 •

edited

Loading

wxtim May 27, 2021

wxtim May 27, 2021

MetRonnie commented May 27, 2021

MetRonnie left a comment

hjoliver commented May 27, 2021 •

edited

Loading

wxtim commented May 27, 2021 •

edited

Loading

wxtim commented May 27, 2021

dpmatthews commented May 28, 2021

MetRonnie commented May 28, 2021 •

edited

Loading

wxtim commented May 29, 2021 •

edited

Loading

wxtim commented May 31, 2021

MetRonnie left a comment •

edited

Loading

wxtim commented Jun 1, 2021

oliver-sanders commented Jun 4, 2021

wxtim commented Jun 11, 2021

oliver-sanders commented Jun 11, 2021

wxtim commented Jun 11, 2021

oliver-sanders commented Jun 11, 2021

		suite_info = self._db_exec(
		user_name, suite_name, 'SELECT * FROM suite_params', []

Problem: replacement of "suite" with "workflow" breaks review. #4233

Problem: replacement of "suite" with "workflow" breaks review. #4233

Conversation

wxtim commented May 27, 2021 • edited Loading

wxtim May 27, 2021

Choose a reason for hiding this comment

wxtim May 27, 2021

Choose a reason for hiding this comment

MetRonnie commented May 27, 2021

MetRonnie left a comment

Choose a reason for hiding this comment

hjoliver commented May 27, 2021 • edited Loading

wxtim commented May 27, 2021 • edited Loading

wxtim commented May 27, 2021

dpmatthews commented May 28, 2021

MetRonnie commented May 28, 2021 • edited Loading

wxtim commented May 29, 2021 • edited Loading

wxtim commented May 31, 2021

MetRonnie left a comment • edited Loading

Choose a reason for hiding this comment

wxtim commented Jun 1, 2021

oliver-sanders commented Jun 4, 2021

wxtim commented Jun 11, 2021

oliver-sanders commented Jun 11, 2021

wxtim commented Jun 11, 2021

oliver-sanders commented Jun 11, 2021

wxtim commented May 27, 2021 •

edited

Loading

hjoliver commented May 27, 2021 •

edited

Loading

wxtim commented May 27, 2021 •

edited

Loading

MetRonnie commented May 28, 2021 •

edited

Loading

wxtim commented May 29, 2021 •

edited

Loading

MetRonnie left a comment •

edited

Loading