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

set default log file by job outcome #2025

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

markgrahamdawson
Copy link
Contributor

@markgrahamdawson markgrahamdawson commented Dec 10, 2024

This pr now implements setting default log file by job outcome in two ways:

  1. If the user navigates to the log view through the node menu the job state is passed to the view as in this pr...
    master...MetRonnie:cylc-ui:menu-log-file

  2. If the user manually types in the path of the job via the input in the log view a new query is made as in this pr...
    offline data: enable querying failed jobs cylc-uiserver#657

Closes #1241

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).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • Changelog entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

src/views/Log.vue Outdated Show resolved Hide resolved
src/views/Log.vue Outdated Show resolved Hide resolved
@markgrahamdawson markgrahamdawson marked this pull request as ready for review February 4, 2025 13:17
fix

re-implemented based on review

fix broken E2E test

fix tests

review ammends

added change log
@markgrahamdawson markgrahamdawson force-pushed the 1241-log-view-set-default-log-file-by-job-outcome branch from f31427b to 44e4d78 Compare February 5, 2025 09:20
src/views/Log.vue Outdated Show resolved Hide resolved
src/views/Log.vue Outdated Show resolved Hide resolved
src/views/Log.vue Outdated Show resolved Hide resolved
let result
try {
// get the list of available log files
result = await this.$workflowService.apolloClient.query({
Copy link
Member

@oliver-sanders oliver-sanders Feb 5, 2025

Choose a reason for hiding this comment

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

We've got an interface for performing queries like this. It's got a silly name and isn't documented or explained (sorry). The workflow service code is in need of refactor but rather low priority.

I think this should work with no other changes, but give it a try first:

Suggested change
result = await this.$workflowService.apolloClient.query({
result = await this.$workflowService.query2({

Comment on lines +245 to +250
query JobState($id: [ID!]) {
jobs (live: false, workflows: $id) {
id
state
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This isn't requesting the state of a single job, it's requesting the state of all jobs in the entire workflow. This is why you have to filter for the specific job ID in the code later on.

Workflows can accumulate many hundreds of thousands of jobs over their lifetime so this could become a bit of a problem.

There is an ids argument which is supposed to filter this down as well as a tasks argument (which I don't think makes sense in this context). I tried these out in graphql and to my horror found that neither actually do the job 🤦. Dammit!

E.G, here I am filtering by workflow, task and job ID, but I still get two results from two different cycles:

query JobState($workflow_id: [ID!], $task_id: [ID!], $job_id: [ID!]) {
  jobs (live: false, workflows: $workflow_id, tasks: $task_id, ids:$job_id) {
    id
    state
  }
}
{"workflow_id":"generic/run5//20191209T1200Z/foo/", "task_id":"foo", "job_id":"20191209T1200Z/foo/run1"}
{
  "data": {
    "jobs": [
      {
        "id": "generic/run5//20191209T1200Z/foo/01",
        "state": "0"
      },
      {
        "id": "generic/run5//20191210T0000Z/foo/01",
        "state": "0"
      }
    ]
  }
}

This is going to need a bit of work on the backend to resolve.

I think this PR should be using the ids field.

Copy link
Member

Choose a reason for hiding this comment

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

Here's the source of the problem:

https://github.com/cylc/cylc-uiserver/blob/aeddbcb01ca68de01258ccdd1c129614361fdde9/cylc/uiserver/schema.py#L306-L307

It only makes use of the task argument (which does work but can return the same task from multiple cycles and multiple job submissions for the same cycle) and completely ignores the id argument (because this hasn't been implemented yet).

console.warn(err)
return
}
const job = result.data.jobs.find(x => x.id === this.id)
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't have to filter for the job by its ID because there should only be one job returned by the query.

This made me take a look at the query and realise that we're getting all jobs back in the response, dammit.

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

Successfully merging this pull request may close these issues.

log view: set default log file by job outcome
3 participants