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

Infer run number & centralised reg parse #4285

Merged
merged 24 commits into from
Aug 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
fe9418b
Fix mixed use of alias in cylc cycle-point help
MetRonnie Jun 10, 2021
2fbfda7
Centralise both parsing of REG and inference of run number
MetRonnie Jun 30, 2021
50cbe95
Type annotations & homogenise "[OPTIONS] ARGS" in script docstrings
MetRonnie Jun 30, 2021
70673f1
Fix functional tests
MetRonnie Jun 30, 2021
007c3e2
Update changelog
MetRonnie Jun 30, 2021
1a9e119
Remove unecessary require_flow_file arg from workflow_files.parse_reg()
MetRonnie Jul 1, 2021
135ab1a
Merge branch 'master' into centralised-reg-parse
MetRonnie Jul 15, 2021
7c1ce36
Create get_cylc_run_dir() function
MetRonnie Jul 16, 2021
7c6721d
Allow parse_reg to use relative paths when src=True
MetRonnie Jul 16, 2021
e26da36
Centralise workflow_files messages; clarify reg clash msg
MetRonnie Jul 16, 2021
7f6db31
Revert "Fix functional tests"
MetRonnie Jul 16, 2021
653f52b
Merge branch 'master' into centralised-reg-parse
MetRonnie Jul 19, 2021
dc58221
Fix use of get_template_vars() post-deconfliction
MetRonnie Jul 16, 2021
57be420
Make add_cylc_rose_options() a method of CylcOptionParser
MetRonnie Jul 16, 2021
ed0845f
Remove now-defunct functional test
MetRonnie Jul 19, 2021
4122314
Merge branch 'master' into centralised-reg-parse
MetRonnie Jul 21, 2021
faa9bcc
Improve coverage exclusion
MetRonnie Jul 21, 2021
9b68a87
Merge branch 'master' into tmp
MetRonnie Aug 4, 2021
fc44f5d
Update changelog
MetRonnie Aug 4, 2021
d1d44c9
Address code review
MetRonnie Aug 10, 2021
32f8f64
Merge branch 'master' into centralised-reg-parse
MetRonnie Aug 12, 2021
8fdec12
Clarify warning msg in response to code review
MetRonnie Aug 13, 2021
a932ce6
Tweak reinstall to partially use centralised reg parse
MetRonnie Aug 13, 2021
56cc652
Merge branch 'master' into centralised-reg-parse
MetRonnie Aug 16, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ graph visualisation.
Have validation catch erroneous use of both `expr => bar` and `expr => !bar` in
the same graph.

[#4285](https://github.com/cylc/cylc-flow/pull/4285) - Cylc now automatically
infers the latest numbered run of the workflow for most commands (e.g. you can
run `cylc pause foo` instead of having to type out `foo/run3`).

[#4346](https://github.com/cylc/cylc-flow/pull/4346) -
Use natural sort order for the `cylc scan --sort` option.

Expand All @@ -74,6 +78,9 @@ Use natural sort order for the `cylc scan --sort` option.
[#4341](https://github.com/cylc/cylc-flow/pull/4341 -
Remove obsolete Cylc 7 `[scheduling]spawn to max active cycle points` config.

[#4319](https://github.com/cylc/cylc-flow/pull/4319) -
Update cylc reinstall to skip cylc dirs work and share

[#4289](https://github.com/cylc/cylc-flow/pull/4289) - Make `cylc clean`
safer by preventing cleaning of dirs that contain more than one workflow
run dir (use `--force` to override this safeguard).
Expand Down Expand Up @@ -136,9 +143,6 @@ respectively.

### Fixes

[#4319](https://github.com/cylc/cylc-flow/pull/4319) -
Update cylc reinstall to skip cylc dirs work and share #4319

[#4296](https://github.com/cylc/cylc-flow/pull/4296) -
Patches DNS issues with newer versions of Mac OS.

Expand Down
22 changes: 12 additions & 10 deletions cylc/flow/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,12 @@
from copy import copy
from fnmatch import fnmatchcase
import os
from pathlib import Path
import re
import traceback
from typing import (
Any, Callable, Dict, List, Mapping, Optional, Set, TYPE_CHECKING, Tuple
Any, Callable, Dict, List, Mapping, Optional, Set, TYPE_CHECKING, Tuple,
Union
)

from metomi.isodatetime.data import Calendar
Expand Down Expand Up @@ -153,8 +155,8 @@ class WorkflowConfig:
def __init__(
self,
workflow: str,
fpath: str,
options: Optional['Values'] = None,
fpath: Union[Path, str],
options: 'Values',
template_vars: Optional[Mapping[str, Any]] = None,
is_reload: bool = False,
output_fname: Optional[str] = None,
Expand All @@ -171,7 +173,7 @@ def __init__(
self.mem_log = lambda x: None
self.mem_log("config.py:config.py: start init config")
self.workflow = workflow # workflow name
self.fpath = fpath # workflow definition
self.fpath = str(fpath) # workflow definition
self.fdir = os.path.dirname(fpath)
self.run_dir = run_dir or get_workflow_run_dir(self.workflow)
self.log_dir = log_dir or get_workflow_run_log_dir(self.workflow)
Expand All @@ -183,15 +185,15 @@ def __init__(
'SequenceBase', Set[Tuple[str, str, bool, bool]]
] = {}
self.taskdefs: Dict[str, TaskDef] = {}
self.initial_point: Optional['PointBase'] = None
self.start_point: Optional['PointBase'] = None
self.final_point: Optional['PointBase'] = None
self.clock_offsets = {}
self.expiration_offsets = {}
self.ext_triggers = {} # Old external triggers (client/server)
self.xtrigger_mgr = xtrigger_mgr
self.workflow_polling_tasks = {} # type: ignore # TODO figure out type

self.initial_point: 'PointBase'
self.start_point: 'PointBase'
Comment on lines +194 to +195
Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies if I've missed the explanation on this one, is there a reason why these are both no longer optional?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't explain; it's because they should always be set by

def process_initial_cycle_point(self) -> None:
and
def process_start_cycle_point(self) -> None:

(start_point takes on value of initial_point if not specified)

This stops mypy errors complaining that start_point or initial_point may be None when trying to use PointBase methods later.

Only problem is, if they somehow don't end up getting set, we'd end up with AttributeErrors, which won't get picked up by mypy. But in that case, we'd still get TypeErrors if they were set to None in __init__ like before, I guess. Not sure what the best practice is, but at least this silences mypy.

self.final_point: Optional['PointBase'] = None
self.sequences: List['SequenceBase'] = []
self.actual_first_point: Optional['PointBase'] = None
self._start_point_for_actual_first_point: Optional['PointBase'] = None
Expand Down Expand Up @@ -594,7 +596,7 @@ def process_cycle_point_tz(self):
)
self.cfg['scheduler']['cycle point time zone'] = orig_cp_tz

def process_initial_cycle_point(self):
def process_initial_cycle_point(self) -> None:
"""Validate and set initial cycle point from flow.cylc or options.

Sets:
Expand Down Expand Up @@ -642,7 +644,7 @@ def process_initial_cycle_point(self):
f"Initial cycle point {self.initial_point} does not meet "
f"the constraints {constraints}")

def process_start_cycle_point(self):
def process_start_cycle_point(self) -> None:
"""Set the start cycle point from options.

Sets:
Expand Down Expand Up @@ -674,7 +676,7 @@ def process_start_cycle_point(self):
# Start from the initial point.
self.start_point = self.initial_point

def process_final_cycle_point(self):
def process_final_cycle_point(self) -> None:
"""Validate and set the final cycle point from flow.cylc or options.

Sets:
Expand Down
4 changes: 2 additions & 2 deletions cylc/flow/network/scan.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
ClientTimeout,
WorkflowRuntimeClient,
)
from cylc.flow.pathutil import get_workflow_run_dir
from cylc.flow.pathutil import get_cylc_run_dir
from cylc.flow.rundb import CylcWorkflowDAO
from cylc.flow.workflow_files import (
ContactFileFields,
Expand Down Expand Up @@ -162,7 +162,7 @@ async def scan(
dict - Dictionary containing information about the flow.
"""
cylc_run_dir = Path(get_workflow_run_dir(''))
cylc_run_dir = Path(get_cylc_run_dir())
if not run_dir:
run_dir = cylc_run_dir
if not scan_dir:
Expand Down
39 changes: 39 additions & 0 deletions cylc/flow/option_parsers.py
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,45 @@ def add_std_options(self):
dest="icp",
)

def add_cylc_rose_options(self) -> None:
"""Add extra options for cylc-rose plugin if it is installed."""
try:
__import__('cylc.rose')
except ImportError:
return
self.add_option(
"--opt-conf-key", "-O",
help=(
"Use optional Rose Config Setting "
"(If Cylc-Rose is installed)"
),
action="append",
default=[],
dest="opt_conf_keys"
)
self.add_option(
"--define", '-D',
help=(
"Each of these overrides the `[SECTION]KEY` setting in a "
"`rose-suite.conf` file. "
"Can be used to disable a setting using the syntax "
"`--define=[SECTION]!KEY` or even `--define=[!SECTION]`."
),
action="append",
default=[],
dest="defines"
)
self.add_option(
"--rose-template-variable", '-S',
help=(
"As `--define`, but with an implicit `[SECTION]` for "
"workflow variables."
),
action="append",
default=[],
dest="rose_template_vars"
)

def parse_args(self, api_args, remove_opts=None):
"""Parse options and arguments, overrides OptionParser.parse_args.
Expand Down
12 changes: 9 additions & 3 deletions cylc/flow/pathutil.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,11 @@


def expand_path(*args: Union[Path, str]) -> str:
"""Expand both vars and user in path, joining any extra args."""
return os.path.expanduser(os.path.expandvars(
"""Expand both vars and user in path and normalise it, joining any
extra args."""
return os.path.normpath(os.path.expanduser(os.path.expandvars(
os.path.join(*args)
))
)))


def get_remote_workflow_run_dir(
Expand All @@ -56,6 +57,11 @@ def get_remote_workflow_run_job_dir(
return get_remote_workflow_run_dir(flow_name, 'log', 'job', *args)


def get_cylc_run_dir() -> str:
"""Return the cylc-run dir path with vars/user expanded."""
return expand_path(_CYLC_RUN_DIR)


def get_workflow_run_dir(
flow_name: Union[Path, str], *args: Union[Path, str]
) -> str:
Expand Down
25 changes: 21 additions & 4 deletions cylc/flow/platforms.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,16 @@
import re
from copy import deepcopy
from typing import (
Any, Dict, Iterable, List, Optional, Tuple, Union, Set)
Any, Dict, Iterable, List, Optional, Tuple, Union, Set, overload
)

from cylc.flow.exceptions import PlatformLookupError, CylcError, NoHostsError
from cylc.flow.cfgspec.glbl_cfg import glbl_cfg
from cylc.flow.hostuserutil import is_remote_host


UNKNOWN_TASK = 'unknown task'

FORBIDDEN_WITH_PLATFORM: Tuple[Tuple[str, str, List[Optional[str]]], ...] = (
('remote', 'host', ['localhost', None]),
('job', 'batch system', [None]),
Expand All @@ -43,6 +46,20 @@
}


@overload
def get_platform(
task_conf: Union[str, None] = None, task_id: str = UNKNOWN_TASK
) -> Dict[str, Any]:
...


@overload
def get_platform(
task_conf: Dict[str, Any], task_id: str = UNKNOWN_TASK
) -> Optional[Dict[str, Any]]:
...


# BACK COMPAT: get_platform
# At Cylc 9 remove all Cylc7 upgrade logic.
# from:
Expand All @@ -53,7 +70,7 @@
# Cylc9
def get_platform(
task_conf: Union[str, Dict[str, Any], None] = None,
task_id: str = 'unknown task'
task_id: str = UNKNOWN_TASK
) -> Optional[Dict[str, Any]]:
"""Get a platform.
Expand Down Expand Up @@ -439,7 +456,7 @@ def fail_if_platform_and_host_conflict(task_conf, task_name):


def get_platform_deprecated_settings(
task_conf: Dict[str, Any], task_name: str = 'unknown task'
task_conf: Dict[str, Any], task_name: str = UNKNOWN_TASK
) -> List[str]:
"""Return deprecated [runtime][<task_name>] settings that should be
upgraded to platforms.
Expand Down Expand Up @@ -555,5 +572,5 @@ def get_random_platform_for_install_target(

def get_localhost_install_target() -> str:
"""Returns the install target of localhost platform"""
localhost = platform_from_name()
localhost = get_platform()
Copy link
Member

Choose a reason for hiding this comment

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

@wxtim can you check this.

Copy link
Member Author

@MetRonnie MetRonnie Jul 1, 2021

Choose a reason for hiding this comment

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

This is just changing it back to how it was before; both identical as can be seen from def get_platform(task_conf=None, task_id=UNKNOWN_TASK):

if task_conf is None or isinstance(task_conf, str):
# task_conf is a platform name, or get localhost if None
return platform_from_name(task_conf)

The reason I had previously changed it to platform_from_name() was because mypy was complaining that localhost might be None due to the Optional return of get_platform(). However this now is fixed by the use of @overload to confirm that if task_conf is None, the return value will definitely be a dictionary

@overload
def get_platform(
task_conf: Union[str, None] = None, task_id: str = UNKNOWN_TASK
) -> Dict[str, Any]:
...

Copy link
Member Author

Choose a reason for hiding this comment

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

I've just found out about a nasty omission in mypy's checking of @overloaded function implementations: python/mypy#9503

return get_install_target_from_platform(localhost)
27 changes: 16 additions & 11 deletions cylc/flow/scheduler_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,12 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.
"""Common logic for "cylc play" CLI."""

from ansimarkup import parse as cparse
MetRonnie marked this conversation as resolved.
Show resolved Hide resolved
import asyncio
from contextlib import suppress
from functools import lru_cache
import os
import sys

from ansimarkup import parse as cparse
from typing import TYPE_CHECKING

from cylc.flow import LOG, RSYNC_LOG
from cylc.flow.exceptions import ServiceFileError
Expand All @@ -43,6 +42,10 @@
from cylc.flow import workflow_files
from cylc.flow.terminal import cli_function

if TYPE_CHECKING:
from optparse import Values


PLAY_DOC = r"""cylc play [OPTIONS] ARGS
Start a new workflow, restart a stopped workflow, or resume a paused workflow.
Expand All @@ -62,7 +65,7 @@
as satisfied.
Examples:
# Start (at the initial cycle point), or restart, or resume workflow REG.
# Start (at the initial cycle point), or restart, or resume workflow REG
$ cylc play REG
# Start a new run from a cycle point after the initial cycle point
Expand All @@ -73,6 +76,10 @@
$ cylc play --start-task=foo.3 REG
$ cylc play -t foo.3 -t bar.3 REG
# Start, restart or resume the second installed run of the workflow
# "dogs/fido"
$ cylc play dogs/fido/run2
At restart, tasks recorded as submitted or running are polled to determine what
happened to them while the workflow was down.
"""
Expand Down Expand Up @@ -231,8 +238,7 @@ def get_option_parser(add_std_opts=False):
}


RunOptions = Options(
get_option_parser(add_std_opts=True), DEFAULT_OPTS)
RunOptions = Options(get_option_parser(add_std_opts=True), DEFAULT_OPTS)


def _open_logs(reg, no_detach):
Expand Down Expand Up @@ -262,7 +268,7 @@ def _close_logs():
handler.close()


def scheduler_cli(parser, options, reg):
def scheduler_cli(options: 'Values', reg: str) -> None:
"""Run the workflow.
This function should contain all of the command line facing
Expand All @@ -273,8 +279,7 @@ def scheduler_cli(parser, options, reg):
functionality.
"""
workflow_files.validate_flow_name(reg)
reg = os.path.normpath(reg)
reg = workflow_files.parse_reg(reg)
try:
workflow_files.detect_old_contact_file(reg)
except ServiceFileError as exc:
Expand Down Expand Up @@ -376,6 +381,6 @@ async def _run(scheduler: Scheduler) -> int:


@cli_function(get_option_parser)
def play(parser, options, reg):
def play(parser: COP, options: 'Values', reg: str):
"""Implement cylc play."""
return scheduler_cli(parser, options, reg)
return scheduler_cli(options, reg)
Loading