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

Add note and block relative #32

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

shaheerk94
Copy link

@shaheerk94 shaheerk94 commented Dec 12, 2024

Closes #30 by adding new note construct and BLOCK_RELATIVE tag type.

Added note and BLOCK_RELATIVE to tests, which are passing.

Also updated owner to @cartermak instead of Chris, since Chris is gone, and version.

Closes #24 by conditionally enforcing tag field on all time types except COMMAND_COMPLETE

@shaheerk94 shaheerk94 requested a review from a team as a code owner December 12, 2024 21:30
@tloreilly56
Copy link

How about REQUEST_RELATIVE? request and ground activity allow steps with time relation relative to the START_TIME of the request. Similar to BLOCK_RELATIVE.

@cartermak
Copy link
Member

@tloreilly56 does BLOCK_RELATIVE have a different existing meaning? The intent is for BLOCK_RELATIVE to be a generalization of SASF FROM_REQUEST_START.

@tloreilly56
Copy link

I assume BLOCK_RELATIVE is equivalent to FROM_ACTIVITY_START, which is the start of the ground block. FROM_REQUEST_START is relative to the start of the request. Suppose in a request you have two ground activities and the steps within each activity have references to FROM_ACTIVITY_START. The relative time of FROM_ACTIVITY_START is different within each activity, each relative to the start of the two ground blocks in the expansion.

@cartermak
Copy link
Member

Aha, thank you. As I understand, SeqJSON doesn't support activities, so a SeqJSON request may only have a flat list of steps and that wouldn't apply unless we added activities. So maybe the question is, do we ever need SeqJSON to support activity expansion? I'm inclined to say no, but maybe it should?

@tloreilly56
Copy link

@cartermak got it. If activity is not going to be supported in seq.json, we don't need to support a time type relative to the activity. For request, we need a time type relative to request start. Naming it REQUEST_START may be less confusing and have BLOCK_START available for later when activity is supported.

@shaheerk94
Copy link
Author

@tloreilly56 Why not use BLOCK_RELATIVE for request now and also be able to use it for activity later if we need to? I don't think we would ever be in a context where are activities and requests, so BLOCK_RELATIVE can mean the start of the request if it's in a request, the start of an activity if it's in an activity, and the start of a sequence if it's in a sequence but not inside a request (like satf). Agree with Carter that I don't expect us to ever support activity block types in seq json, though.

@tloreilly56
Copy link

I understand we never expect to support activity block in seq.json. However, a request in seq.json can have a ground_block step which refers to a ground activity outside seq.json. That ground activity can have FROM_ACTIVITY_START or FROM_REQUEST_START time tag in the steps. I think using BLOCK_RELATIVE for steps in requests to refer to the start of the request is confusing.

@shaheerk94
Copy link
Author

shaheerk94 commented Dec 17, 2024

@tloreilly56 The idea is that we can use BLOCK_RELATIVE as a general time tag to capture any time of time tag where the step should be issued relative to its container. The container could be an activity, a request, a sequence, etc. but BLOCK_RELATIVE can be used the same way in each case. In other words, how BLOCK_RELATIVE is used will depend on the context in which it is used. The issue is that if we use REQUEST_RELATIVE, then REQUEST_RELATIVE can only be a valid time tag within request types, and trying to enforce that REQUEST_RELATIVE can only be used for steps inside requests is going to complicate the schema. So we're trying to use a generalized time tag that for now is only going to be used for FROM_REQUEST_START but later could be used for other, similar types of time tags that may come up.

More generally, I would like to move away from having everything in SEQ JSON be 1:1 with what was in SATF/SASF

Does that make sense?

In your example, the request would only have a call to a ground block step, and the ground block would be entirely separate, so BLOCK_RELATIVE should have two distinct meanings in both cases. Also, as you pointed out, we do not plan to support ground blocks in seq json. So the meaning of BLOCK_RELATIVE in seq json should always be clear.

@tloreilly56
Copy link

@shaheerk94 what I was trying to point out is in a ground block you could have a time tag relative to start of the block (BLOCK_RELATIVE) or relative to the start of the request (REQUEST_RELATIVE) that calls the block. Yes, in the request, you could use BLOCK_RELATIVE to represent the start of the request. I just think user might find it confusing using the term BLOCK_RELATIVE to represent relative to the start of request.

schema.json Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@cartermak
Copy link
Member

Attendees

@shaheerk94
@parkerabercrombie
@dandelany
@tloreilly56
@cartermak

Minutes

  • In SEQ 2.0, we aren't carrying forward all existing terminology. SEQ 2.0 doesn't have a meaning to "block" outside VML, so here it could mean just a chunk of commands - sequence, request, ground block, etc.
  • SeqJSON doesn't support definition of ground blocks, but you can call a ground block. So there could be ambiguity in the event of nested blocks where the "block" from which start time is being referenced is unclear.
  • A ground block definition (from outside SeqJSON) can have a time tag which means something different than what we're proposing it means in SeqJSON. That time tag in SATF would be FROM_ACTIVITY_START, which is colloquially understood to mean "from the start of this block".
  • We don't need to preserve terminology from legacy formats necessarily.
  • We aren't changing the existing understanding of "block relative", we're just extending it to apply to requests.
  • Seqgen does have an existing meaning of "block", and it doesn't include requests. So this does constitute more of a breaking change in the Seqgen context, even if the term isn't well-established in SeqJSON.
  • Takeaway: disagreement is purely with overloading "block". Group is in agreement that we can have a time tag which means "relative to the start of this execution context", be it request, sequence, block, etc. We want something like "container relative", although it would be nice to have an initialism that's distinct from our existing time types (A, C, E, R). Other ideas: parent-relative, batch-relative, nested-relative, scope-relative, local-relative. "Parent" could be mis-interpreted as referencing the calling entity (e.g., parent sequence). D, T, P prefer "scope-relative", but leaving the door open to other suggestions.
  • Next steps: mull over exact terminology, then update PR and request PMC approval.
  • Side Q: What is a block? Both requests and blocks can contain logic and commands, but requests are higher-level than blocks.
  • Side Q: How to time request steps relative to a specific epoch? Currently, we can only do this in SeqJSON when timing the request start. Some users may want to define the values for epochs in their sequence files too (heritage from SASF).

@cartermak
Copy link
Member

cartermak commented Dec 19, 2024

Some examples to describe the time type behavior.

This first example shows how multiple block-relative steps in an absolute-timed request would be evaluated:

{
    "id": "example",
    "metadata": {},
    "requests": [
        {
            "description": "This is a request",
            "name": "test_request_1",
            "steps": [
                {
                    "description": "This command executes 10s after the request start at 2020-001T00:00:10",
                    "args": [],
                    "time": {
                        "type": "BLOCK_RELATIVE",
                        "tag": "00:00:10"
                    },
                    "stem": "CMD_NO_OP",
                    "type": "command"
                },
                {
                    "description": "This note is evaluated 20s after the request start at 2020-001T00:00:20",
                    "string_arg": "Note Test.",
                    "time": {
                        "type": "BLOCK_RELATIVE",
                        "tag": "00:00:20"
                    },
                    "type": "note"
                },
                {
                    "description": "This command executes 30s after the request start at 2020-001T00:00:30",
                    "args": [],
                    "time": {
                        "type": "BLOCK_RELATIVE",
                        "tag": "00:00:30"
                    },
                    "stem": "CMD_NO_OP",
                    "type": "command"
                }
            ],
            "time": {
                "tag": "2020-001T00:00:00",
                "type": "ABSOLUTE"
            },
            "type": "request"
        }
    ]
}

This second example shows how the timing of these sequence steps would depend on modeling context of the sequence (I'm not sure there's any heritage for this pattern, but one could imagine the modeling software resolving these times to, e.g., absolute time tags for commands):

{
    "id": "mixed time types",
    "metadata": {},
    "steps": [
        {
            "description": "This command executes at 2020-001T00:00:00",
            "args": [],
            "time": {
                "type": "ABSOLUTE",
                "tag": "2020-001T00:00:00"
            },
            "stem": "CMD_NO_OP",
            "type": "command"
        },
        {
            "description": "This command executes 10s after the sequence start",
            "args": [],
            "time": {
                "type": "BLOCK_RELATIVE",
                "tag": "00:00:10"
            },
            "stem": "CMD_NO_OP",
            "type": "command"
        },
        {
            "description": "This command executes after the previous command completes",
            "args": [],
            "time": {
                "type": "COMMAND_COMPLETE"
            },
            "stem": "CMD_NO_OP",
            "type": "command"
        },
        {
            "description": "This command executes 30s after the previous command start",
            "args": [],
            "time": {
                "type": "COMMAND_RELATIVE",
                "tag": "00:00:30"
            },
            "stem": "CMD_NO_OP",
            "type": "command"
        },
        {
            "description": "This command executes 1h after the sequence start",
            "args": [],
            "time": {
                "type": "BLOCK_RELATIVE",
                "tag": "01:00:00"
            },
            "stem": "CMD_NO_OP",
            "type": "command"
        }
    ]
}

@cartermak
Copy link
Member

Sharing here a sort of "majority opinion" on the BLOCK_RELATIVE terminology, composed mostly by Shaheer.

So we’re all in agreement that we want a generalized time tag that means “a time relative to the start of my container,” where “container” is some organizational construct and can include onboard subroutines, ground subroutines, reusable sequences, requests, etc. This time tag needs a name and it would be great if we could use CONTAINTER_RELATIVE, but unfortunately “C” is already accounted for in SeqN. We see two options, barring breaking changes to existing SeqN keywords/syntax:

  1. Co-opt and broaden the existing term "block" to generally mean "container of commands" and choose BLOCK_RELATIVE as our new time tag.
  2. Define a new term such as SCOPE_RELATIVE or START_RELATIVE.

A couple things to consider:

  • Although the term “block” has meaning in VML, it lacks formal definition in SEQGEN or SeqJson. The term is used in a few places: RT_on_board_block refers to onboard subroutines and “ground block” is colloquially used to describe SEQGEN SATF activity types. Note that the terms “block” or “ground block” aren't actually used in the syntax of a SEQGEN ground block.
  • SEQ 2.0 has avoided using the term “block” so far, instead opting to use “onboard subroutine,” defined as: “Any form of command encapsulation that lives onboard the spacecraft and can be invoked by command. Examples include VML blocks, onboard reusable sequences, or semi-permanent macros.” So, in the context of SEQ 2.0 and the associated SeqN and SeqJson formats, "block" is available for redefinition.

Option 1 is essentially using "block" to capture the SEQ 2.0 construct of "onboard subroutine" plus other organizational structures like requests.

Reasons to prefer Option 1 over Option 2:

  • Users will find it easier to adjust their current understanding of the term “block” to include organizational structures such as requests than trying to learn a new, broad term.
  • We should pick an option that has as few ambiguous interpretations as possible. I think the term “block” more readily lends itself to mean things such as requests, sequences, onboard blocks, etc., than “scope” or “start”. Equally unambiguous is “container”, but-
  • We should pick an option that has an intuitive letter than can be used in SeqN. Option 2 satisfies the previous point, but “C” and “R” are both unavailable.
  • More broadly, we should feel empowered to adopt and "own" sequencing terminology, especially terms that currently lack clear definitions.

In response to specific points against using "block" here:

  • Point: We want to avoid using the term BLOCK_RELATIVE is because of historical associations/connotations with the word block, and because in SEQGEN, a “block” conceptually means a subtly different thing that what we would be proposing to use it for here.
  • Counterpoint: I think that the ambiguity in the definition of the term “block” and existing user expectations of the term actually presents us an opportunity take an existing term and give it a formal, standard definition. I think that building upon existing understanding and familiar terminology will ultimately lead to less confusion than trying to introduce completely new terms that users need to learn from scratch.
  • Point: In the event of, e.g., a ground block, a user may want to time relative to either the start of the ground block or relative to the start of the request which calls that ground block.
  • Counterpoint: This is a confusing capability. In the event of nested requests, the behavior (assuming it's even well-defined) would be unintuitive, and it seems like poor design for an entity to reference characteristics this far outside its own scope. More broadly, we shouldn't be afraid to simply our core constructs where reasonable.

@ewferg ewferg self-requested a review January 3, 2025 04:47
Copy link
Collaborator

@ewferg ewferg left a comment

Choose a reason for hiding this comment

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

While I did not review the code itself, I approve the use of BLOCK_RELATIVE and the addition of a "note" step based on the nice write up above.

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