-
Notifications
You must be signed in to change notification settings - Fork 41
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
Move time-sync to builtin feature #822
Comments
TimeSync
so that it accepts config from the top level manifest context
TimeSync
accepts config from top level context
TimeSync
accepts config from top level contextTimeSync
to accept config from top level context
TimeSync
to accept config from top level context
@manushak @narekhovhannisyan please review ac to confirm you're ok with it |
note that there is a dependency in #842 which will mean that during the implementation of this feature we need to review the aggregate feature and remove temp workaround code |
expecting a PR today |
need to fix the issues reported during the review |
@manushak @MariamKhalatova please take a look at the linked PR 🙏 |
as discovered by running converted manifests for phased execution, it turns out that having this as a built in feature causes a lot of unwanted side effects for other plugins and to address all of them would require changing multiple plugins. the safest option for now is to revert back to it being a plugin. |
As I understood from @jawache this one can be backlogged for the time being until we have more experience making real life manifests and better awareness of the related frictions |
Why: Sub of #763
What: Refactor
TimeSync
so that it is an IF builtin feature and not a plugin. It accepts config from the top level manifestcontext
and is automatically executed afterregroup
if the config exists.Blocked by:
observe
,regroup
andcompute
as distinct execution phases #809Scope of work:
TimeSync
so that it is an IF core feature rather than a plugincontext
portion of a manifest) instead of in the plugininitialize
fieldTimeSync
config exists and automatically executes betweenregroup
andcompute
Acceptance criteria
Time Sync is an IF builtin feature
GIVEN TimeSync is moved
WHEN I browse the IF repository
THEN I should NOT find
/src/builtins/time-sync.ts
but I SHOULD findsrc/lib/time-sync.ts
Time Sync does not require intiializing in a manifest, and its config comes from manifest
context
GIVEN TimeSync is moved to a core feature
WHEN I run
if-run -m demo.yml
AND demo.yml contains the following:
THEN it runs successfully and the output is:
GIVEN the check is implemented
WHEN I run the following manifest (first timestep is duplicated)
THEN TimeSync throws an exception , such as
TimeSeriesError: Duplicaste timestamps exist in <path-to-node>
regroup
and immediately beforecompute
GIVEN The changes are implemented
WHEN I run a manifest that includes valid TimeSync config and
regroup
config in thecontext
THEN IF should execute
observe
thenregroup
thentimesync
thencompute
e.g. this manifest should trigger the intended behaviour
The text was updated successfully, but these errors were encountered: