-
Notifications
You must be signed in to change notification settings - Fork 93
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
global universal identifier #3931
Conversation
@dwsutherland I think this will marry well with the pre-existing universal identifier. Tagged you as the original author of the universal identifier to make sure I haven't thrown in any curveballs. The only thing that I think will trip it up at the moment is the ability to specify selectors on cycle points (as opposed to tasks) though I'm not entirely sure what that would mean in different contexts and we can block that functionality for now. |
cylc/flow/id.py
Outdated
Returns: | ||
list - List of tokens dictionaries. | ||
|
||
Examples: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These examples show the possible formats and upgrading of legacy ids.
@oliver-sanders - Really amazing work, I wish I was as sophisticated as you!
Selectors on cycle points would probably target the root family of that cycle-point (from an API perspective) Also have you thought about the APIs use of the following ID format? Also, in case you/others need a reminder, |
Join the club - I think it's the "barnyard strength" cider he runs on 🍺 |
Aw shucks, didn't know there was so much love for regexes
Could work, seemed like a good idea at the time but it gets a little bit weird:
Can use a wildcard
I think in the cases where you might want to specify a task without a cycle we use the
Nice. The other thing I was thinking about was multi-user support. Users would not need to log into group accounts to control workflows e.g:
And also admin privileges e.g:
The command would be sent to the UIS for "alice" where it would authenticate as me and check authorisation as it would for a command from the UI. Especially nice as Cylc Flow would not need to provide any multi-user support, it just piggy-backs on the UIS which already has to do this stuff for the UI. |
Untested cycle/task matching for the task_pool (replaces the def filter_ids(self, ids, out=Tokens.Task):
match = fnmatchcase
# TODO: consider using wcmatch which would add support for
# extglobs, namely brace syntax e.g. {foo,bar}
_cycles = []
_tasks = []
_jobs = []
_not_matched = []
for tokens in ids:
for lowest_token in reversed(Tokens):
if tokens.get[lowest_token.value]:
break
cycles = []
tasks = []
jobs = []
# filter by cycle
if lowest_token == Tokens.Cycle:
cycle = tokens[Tokens.Cycle.value]
cycle_sel = tokens.get(Tokens.Cycle.value + '_sel', '*')
for icycle, itasks in self.pool.items():
str_cycle = str(icycle)
if not match(str_cycle, cycle):
continue
if cycle_sel == '*':
cycles.append(str_cycle)
continue
for itask in itasks:
if match(itask.state.status, cycle_sel):
cycles.append(icycle)
break
# filter by task
elif lowest_token == Tokens.Task:
cycle = tokens[Tokens.Cycle.value]
cycle_sel = tokens.get(Tokens.Cycle.value + '_sel', '*')
task = tokens[Tokens.Task.value]
task_sel = tokens.get(Tokens.Task.value + '_sel', '*')
for icycle, itasks in self.pool.items():
str_cycle = str(icycle)
if not match(str_cycle, cycle):
continue
if cycle_sel == '*':
cycles.append(str_cycle)
continue
for itask in itasks:
if (
match(itask.state.status, cycle_sel)
and match(itask.tdef.name, task)
and (
match(itask.state.status, task_sel)
or any(
match(ns, task)
for ns in itask.tdef.namespace_hierarchy
)
)
):
tasks.append(itask)
else:
raise NotImplementedError
if not cycles or tasks or jobs:
_not_matched.append(tokens)
else:
_cycles.extend(cycles)
_tasks.extend(tasks)
_jobs.extend(jobs)
ret = None
if out == Tokens.Cycle:
_cycles.extend([
itask.point
for itask in _tasks
])
ret = _cycles
elif out == Tokens.Task:
_tasks.extend([
self.pool[icycle]
for icycle in _cycles
])
ret = _tasks
return ret, _not_matched |
👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty happy with these changes. Only a couple questions:
- I guess the use of
//
&/
is the best we can do? (slashes seem to be used in many places.. and even the double works in paths.. Can it be overused? or is it the URL safe choice) - I suppose there's no way to centralise
//
&/
it can be changed easily? (i.e. f-string substitution inid.py
)
Apologies if the discussion has already taken place (i.e. Proposal) on the above!
PS - If only there was a better way to pattern match selection, but I guess there's no avoiding the looping:
for icycle, itasks in self.pool.items():
str_cycle = str(icycle)
if not match(str_cycle, cycle):
continue
if cycle_sel == '*':
cycles.append(str_cycle)
continue
for itask in itasks:
at least its only going through each task once I suppose O(n)... (same with non ID lookup resolvers)
Good point about URL safety, will come back to after the beta release. |
I think the use of Here's the rationale behind using two characters: # chars considered:
/ ! @ % ^ & * ( ) - _ = + [ ] { } / | ; : , . < > ~ #
# conflicts with hierarchical workflow registrations
/
# conflicts with glob chars in the shell and the :sel pattern
[ ] { } *
# conflicts with the :sel prefix
:
# conficts with shell
; ( ) < > | & #
# conflicts with ~user prefix
~
# conflict with workflow name
/ _ + - . @
# conflict with task name
- + % @
# conflicts with extglob (shell)
!
# partial conflict with shell (substitution?)
^
# chars left:
=, So that just leaves us with the following two char combinations: //
/=
=/
/,
,/ https://github.com/cylc/cylc-admin/blob/master/docs/proposal-universal-id.md |
Not |
Possible, but a bit strange:
|
(on draft waiting for #4230 which will provide the place for this argument parsing to be inserted) |
This comment has been minimized.
This comment has been minimized.
That's a broken import in cylc-uiserver which is fixed in cylc/cylc-uiserver#297 |
Yikes, I merged @wxtim's log-message tweak branch, and now we have 300 conflicting files here 😱 |
Another reason this should go in ASAP 😅 |
Yeah, currently attempting to fix the conflicts myself... vim macros to the rescue |
d1ce0fe
to
164dd49
Compare
@oliver-sanders - I merged from master and deconflicted, and pushed a couple of commits to your branch. Looks like all the tests will pass, except for test-tutorial-workflow. Are you particularly attached to your ~90-commit history on this branch, or happy to have it squashed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did lots of manual testing. No problems found during quick code sanity check (thanks @MetRonnie and @dwsutherland for doing the real work 😁 )
Feel free to merge this... |
Will see if Oliver is happy with the squash.. I guess |
Thanks, managed to get the diff coverage up to ~90% which is good but glad to have some heavy manual testing!
I still have one comment from Ronnie to address about metavars in the CLI. There appear to be three forms remaining, my best attempt ATM:
No (checkbox unticked in desc) and we should not merge PRs with messy commits like this (boats the repo and makes it very hard to track down bugs). I'm going to squash at the end, hadn't done it yet to make review easier. |
I'll address the remaining comments, rebase squash and push shortly. |
Dammit, I don't think I can rebase-squash because of the merge commit, may have to squash merge. |
Ok, last feedback comments addressed, tests willing that should do it. @MetRonnie can you once over the docs changes in this commit - 76770ea - which standardises the CLI metavars and adds more docs. |
Agreed, that's why I did not merge, and asked - I expected you still wanted to do some history rationalization. |
Mysterious functional test failures disappeared when I reran the tests. 🎉 Right, this bad boy is going in... |
@oliver-sanders - I'll squash and merge, to a single commit. It doesn't sound as if you wanted to do more complex history surgery to end up with |
Thanks all for reviewing, that was a horrible one. |
closes #3592
Convert cylc-flow to use the global universal identifier
Overview of UID:
~user/workflow//cycle/task/job
.//cycle/task/job
.cylc graph --reference
andcylc play --reference-log
output.The universal ID replaces/unifies:
user|workflow|cycle|task|job
user|workflow|task-definition
task.cycle
task
cycle/task
cycle
cycle/task/job
There should now only be one interface for parsing to and from string IDs as well as a standard
tokens dictionary format for passing around parsed ID information.
Here are some of the parsing interfaces removed in this PR:
It also unifies the pattern matching in the:
There is plenty of scope for future work better adopting the new tokens internal
interchange format to reduce the amount of needless parsing to / from string IDs.
CLI Overview:
Behaviour Changes:
cylc validate foo
could mean./foo
or~/cylc-run/foo
../foo
you must type./foo
.*
).via
cylc scan
.cylc stop '*'
!cylc show
must now be provided with the--task-def
argument.cylc hold
.//*/<name>
:*
for the cycle point).//!namespace
Terminology & Variable Names:
UID gives us a good opportunity to standardise naming conventions, the UID scheme suggests the following:
Variable names:
--user
(Cylc used to have multi-user functionality).CLI:
Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.