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

Centralise general id delimiter #3625

Merged
merged 1 commit into from
May 27, 2020

Conversation

dwsutherland
Copy link
Member

@dwsutherland dwsutherland commented May 27, 2020

This PR:

  • Centralises the ID delimiter.
  • Fixes bug in ID generated in prerequisites dump.
  • Imports to functional tests

Sibling PR: cylc/cylc-uiserver#140
(needs to be merge after this one)

The centralisation/put-at-lower-level is needed due to:

  • A circular import error occurred while trying to fix the prerequisite dump. (ID_DELIM defined in data_store_mgr, but prerequisites is imported to data_store_mgr indirectly)
  • Future proofing.

Justification for cylc/flow/__init__.py location:

  • Not only used by tasks (i.e. shouldn't go in cylc/flow/task_id.py), used for workflow/jobs/families ...etc.
  • Used by multiple modules (i.e. tasks/jobs/data-store/..etc) and UIS, but also as we become more event driven (i.e. the change in state might create a data element with only fields changed which gets merged locally and sent).
  • If we decide on a delimiter for external use that jumps through the def/datetime/shell CLI/Cylc constraints then it can be used internally too (which means more reliance on centralized ID_DELIM). See Universal suite/task/cycle delimiter (universal identifier) #3592

Could be it's own module.. But, as a one-liner and used by UIS, cylc.flow seems appropriate.

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).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Already covered by existing tests.
  • No change log entry required (why? e.g. invisible to users).
  • No documentation update required.

@dwsutherland
Copy link
Member Author

The bug was the conditions using a false ID for their taskProxy, so now works:

query {
  workflows(ids: ["sutherlander|baz"]) {
    taskProxies (ids: ["*|bar"]) {
      id
      state
      prerequisites {
        expression
        satisfied
        conditions {
          reqState
          exprAlias
          taskProxy {
            id
          }
        }
      }
    }
  }
}
{
  "data": {
    "workflows": [
      {
        "taskProxies": [
          {
            "id": "sutherlander|baz|20170101T0000+12|bar",
            "state": "waiting",
            "prerequisites": [
              {
                "expression": "c0 | c1",
                "satisfied": false,
                "conditions": [
                  {
                    "reqState": "succeeded",
                    "exprAlias": "c0",
                    "taskProxy": {
                      "id": "sutherlander|baz|20170101T0000+12|foo"
                    }
                  },
                  {
                    "reqState": "succeeded",
                    "exprAlias": "c1",
                    "taskProxy": {
                      "id": "sutherlander|baz|20170101T0000+12|qux"
                    }
                  }
                ]
              }
            ]
          }
        ]
      }
    ]
  }
}

Comment on lines -239 to +240
t_id = f"{workflow_id}/{point}/{name}"
t_id = f"{workflow_id}{ID_DELIM}{point}{ID_DELIM}{name}"
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't matter that this was a different delimiter here? (/ vs |)

Copy link
Member

Choose a reason for hiding this comment

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

(Or was that the bug? it's used in PbConditions)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this was the bug that needed fixed. But can't import from cylc.flow.data_store_mgr.

@hjoliver hjoliver merged commit fe1ed1e into cylc:master May 27, 2020
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.

4 participants