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

rose bush: improve filtering of job states #1155

Closed

Conversation

benfitzpatrick
Copy link
Contributor

This change makes it possible to filter state over all
jobs, as opposed to the old behaviour of filtering state
only on task final job statuses.

This is done by calculating state filtering in Python
rather than via a SQL query.

The state filtering API and GUI is improved to avoid
negative logic.

@matthewrmshin, please test and review.

This change makes it possible to filter state over all
jobs, as opposed to the old behaviour of filtering state
only on task final statuses.

This is done by calculating state filtering in Python
rather than via a SQL query.

The state filtering API and GUI is improved to avoid
negative logic.
@benfitzpatrick benfitzpatrick added this to the next-release milestone Mar 3, 2014
@benfitzpatrick
Copy link
Contributor Author

One niggle is that the new Only: text in the display filters is not
perfectly vertically aligned

@matthewrmshin
Copy link
Member

Some comments:

  • The old functionality is required as well as the new one.
  • The API argument change is going to break user bookmarks.

@matthewrmshin matthewrmshin modified the milestones: next-release, 2014-03 Mar 19, 2014
@benfitzpatrick
Copy link
Contributor Author

The old functionality was hard to understand, which is one of the
reasons for the change. The other reason was that it did not work
as the user expected. Do you really want it back?

Excluding a given state (e.g. no_state=active) can be done by
showing only the other ones (e.g. only_state=success&only_state=fail).
I think this is enough, and it is much clearer.

I agree the API change needs to be announced and the change
needs to be managed well.

@matthewrmshin
Copy link
Member

Do you really want it back?

Apparently so.

Excluding a given state...

And the default is? Previously, in the absence of arguments, the default is to display everything. Hence the negative filter.

@benfitzpatrick
Copy link
Contributor Author

Do you really want it back?

Apparently so.

Which part? no_state, GUI, ...

Excluding a given state...

And the default is? Previously, in the absence of arguments, the default is to display everything. Hence the negative filter.

Um, the default is still to display everything. I can make all the checkboxes ticked
and remove the "Only" text if you want. Would that address your "old functionality"
request as well?

@benfitzpatrick
Copy link
Contributor Author

Have a look at e.g.:
http://www.amazon.co.uk/s/ref=sr_nr_p_89_3?rh=n%3A11052681%2Cn%3A10709121%2Cn%3A376320011%2Cn%3A3028508031%2Cn%3A3028509031%2Ck%3Arose%2Cp_89%3AJustArtificial|Meena+Supplies&keywords=rose&ie=UTF8&qid=1395226333&rnid=1632651031

and the "Brand" filter, which behaves in the same way
as this pull request's filtering as it is now. This kind of
positive filtering is more common because it is clearer
and, in both cases, more aligned with what the user
wants.

@arjclark
Copy link
Contributor

@benfitzpatrick - previous behaviour issues aside, there are a couple of minor issues that will need addressing:

Cycles page:

  • The mouseover for the numbers indicates the number of succeeded/running tasks your filtering works at the job level, so clicking on those buttons does not take you to the equivalent page so there is an interface problem there.
  • The job failures count on the cycles page should now be clickable as your filtering takes you through to those entries
  • The links from the numbers for running, succeeded, failed are now wrong as the number of things you see is not necessarily equivalent to the count you clicked on e.g. task succeeds, is re-run and subsequently fails.

Job listing page:

  • I've lost the ability to tell if a task has succeeded but previously failed - Matt's negative filtering preference aside, being able to filter on tasks on this screen was useful (if not important) to have.

@arjclark
Copy link
Contributor

I have no problem with the tick box filtering being blank at the start of viewing a whole cycle as it is prefaced with the word "Only" so if nothing is selected it implies that the "Only" filter is not being used.

@benfitzpatrick
Copy link
Contributor Author

Job listing page:
I've lost the ability to tell if a task has succeeded but previously failed - Matt's negative filtering preference aside, being able to filter on tasks on this screen was useful (if not important) to have.

I originally implemented it to add all jobs from tasks that contained any fail jobs
and showed it to Jim - his comment was that this behaviour was overly complex
from a user perspective and he would just like failed jobs. I thought about it and
I agree with him.

Fortunately, there is a way of seeing this information.

The most recent job of a task has a black circle, and older jobs are indicated in grey.
So if you are only viewing failed tasks:
❗ my_task (2)
❗ my_task (1)
❗ my_bad_task (2)
❗ my_bad_task (1)

indicates that there is a newer job for my_task that is either active or succeeded.
However, my_bad_task is currently in the failed state.

This can be accessed quickly by clicking on the task name.

We could make this clearer by writing e.g. 1/3, 2/3 in those task job numbers.

@benfitzpatrick
Copy link
Contributor Author

The mouseover for the numbers indicates the number of succeeded/running tasks your filtering works at the job level, so clicking on those buttons does not take you to the equivalent page so there is an interface problem there.

I'm fine with there being a difference between tasks and jobs. There is already
an area of interface confusion. We're talking about referring to the tasks in
cycles.html (which is correct for the user on that page, so I don't want to
change it) but then clicking through to a page called jobs.html. Unless we
make jobs.html act like it was tasks.html (which I do not want, which is
the point of this pull request) then we're going to have a mild presentation
problem. I think the user can cope.

However, I agree that we should do more to make the distinction clear.

In actual fact, the mismatch in presentation does not really apply to
active or succeeded tasks/jobs. This is because of two things:

  • a task very rarely has more than one active or succeeded job
  • when a task has an active or a succeeded job, that job is usually the latest one.

However, it is much more usual for a task to have:

  • more than one failed job
  • a failed job which is not the latest job (i.e. the task has a different current status)

This is the reason for there being a different presentation of statuses on the
cycles.html page for the failed column compared to the active and
succeeded columns. So, I think this problem is already handled.

However, we could introduce a only_latest_job switch if we were really keen.

The links from the numbers for running, succeeded, failed are now wrong as the number of things you see is not necessarily equivalent to the count you clicked on e.g. task succeeds, is re-run and subsequently fails.

What you see is a superset of that information, which is not incorrect, just more
verbose. We could add a jobs number in a similar fashion to the failed column,
to the active and succeeded column, but that would be a bit slower, and it would
nearly always be the same number as the tasks number, as discussed above.

@benfitzpatrick
Copy link
Contributor Author

The job failures count on the cycles page should now be clickable as your filtering takes you through to those entries

Now implemented

@benfitzpatrick benfitzpatrick modified the milestones: next release, 2014-04 Apr 1, 2014
@matthewrmshin matthewrmshin modified the milestones: next release, 2014-05 May 15, 2014
@matthewrmshin matthewrmshin modified the milestones: next-release, 2014.06 Jun 11, 2014
@matthewrmshin matthewrmshin modified the milestones: next-release, 2014.06.1 Jun 20, 2014
@matthewrmshin matthewrmshin modified the milestones: next-release, later Aug 8, 2014
@matthewrmshin
Copy link
Member

See also cylc/cylc-flow#1052.

@matthewrmshin matthewrmshin removed this from the later milestone Nov 13, 2014
@matthewrmshin
Copy link
Member

I'll close this one done as a wontfix for now to reduce the noise. We can revisit this when we have time to concentrate on Rose Bush again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants