Flow profiles#130
Conversation
himslm01
left a comment
There was a problem hiding this comment.
A few readability improvements.
Updated wording Co-authored-by: James Sandford <james.sandford@bbc.co.uk>
… flow-profiles
… flow-profiles
samdbmg
left a comment
There was a problem hiding this comment.
Looks good to me overall: a good approach of maintaining flexibility while constraining in a way to make it easier to use.
I've inlined a handful of suggestions, and a couple of nits
|
I've also kicked off the CI for you, so you should be able to see if it passes validation |
| { | ||
| "type": "object", | ||
| "description": "Describes the technical characteristics of a Flow (common properties to all Flows, imported by type-specific specifications)", | ||
| "title": "Flow Core", |
There was a problem hiding this comment.
NIT: Would be good to have the title updated as it keeps the rendered version more readable, there are currently two schemas rendered out as Flow Core
Co-authored-by: Sam Mesterton-Gibbons <sam@samn.co.uk>
Co-authored-by: Sam Mesterton-Gibbons <sam@samn.co.uk>
… flow-profiles
Co-authored-by: Georgina Shippey <9370167+GeorginaShippey@users.noreply.github.com>
Co-authored-by: Georgina Shippey <9370167+GeorginaShippey@users.noreply.github.com>
… flow-profiles
danjhd
left a comment
There was a problem hiding this comment.
Also this may be a nit and BBC R&D can guide but my feeling is that when referring to TAMS entity types like Flow(s). Source(s) or Profile(s) we should always capitalise these words to help clarify?
|
|
||
| When using the second option on submission of the create flow request, the store will then read the metadata for a given profile and use this to populate the metadata for the flow. | ||
| This de-normalisation of the technical parameters means that on the read side the flows created via both mechanisms have the same technical parameters available. | ||
| Additionally if the decision is made to change a standard profile in the future then this does not affect the existing flows as the parameters have already been replicated and should remain unchanged to reflect the media actually being stored. |
There was a problem hiding this comment.
Above you mention, quite rightly i believe, that Flows should be immutable. Yet here you talk about a use case where a standard profile could be changed. This conflicts. I don't believe we should allow this use case. As you say above if the profile needs to change a new one with a new Id should be created and used.
There was a problem hiding this comment.
Good point - we should remove this as not required. Will re-work this section anyway based on another comment
| 1. Specify all the technical characteristics of the flow including the wrapper, codec and essence parameters alongside the non-technical parameters such as label and description | ||
| 2. Provide just the technical profile for the flow and non-technical parameters required | ||
|
|
||
| When using the second option on submission of the create flow request, the store will then read the metadata for a given profile and use this to populate the metadata for the flow. |
There was a problem hiding this comment.
It feels here like we are making a decision for the implementation? As long as the GET request to the Flow returns all the metadata it should not matter how the implementation does this. i.e. it might not populate them it might just keep the reference to the profile id and "hydrate" on request? The important point is that when GETting Flows a client should be able to see all the metadata regardless of how it was created.
There was a problem hiding this comment.
Re-worked section to hopefully including this
|
|
||
| A profile should be treated as immutable, so once created it cannot be updated. | ||
| Updating a profile would cause mismatches with flows which have been already created using that profile and so breaks the model for profiles. | ||
| To update a profile a new one with a new ID should be created. |
There was a problem hiding this comment.
Do we (or should we) have an option to "archive" an "old" Profile that should no longer be used but is in use already and therefore cannot easily be deleted?
There was a problem hiding this comment.
Currently this is not in scope of this PR. We do need to think about old profiles, however we should also consider the same approach for the storage backend endpoint under the services section also. So previously proposed that we handle both of these at the same time in a future PR
There was a problem hiding this comment.
For completeness, one option is to add fine-grained auth to profiles (and storage backends) to allow their use to be restricted as required.
| Additionally if the decision is made to change a standard profile in the future then this does not affect the existing flows as the parameters have already been replicated and should remain unchanged to reflect the media actually being stored. | ||
|
|
||
| Flows that have been created from a profile will include the parameter indicating which profile they were created from. | ||
| This differentiates them from the flows that have been created with the technical characteristics directly. |
There was a problem hiding this comment.
Do we need to differentiate between flows that were created using a Profile and a Flow created using exactly the same Metadata as an existing Profile?
If we didn't then perhaps the store should detect this at creation time and set the Profile Id to match even if not supplied?
OR it could reject the request saying that this Flow "metadata" matches an existing Profile and therefore the profile should be used?
Just thinking out loud here.
There was a problem hiding this comment.
This probably needs the outcome of the greedy match discussion to complete. If we're not going to implement greedy match at any level then the only way the profile_id field should be populated is if it's provided at creation.
If we want to do match on creation and populate the profile field then yes we may want to indicate this. However the question is what use is this in the actual TAMS metadata or whether it should be captured in an observability layer for reporting/tracking.
I would probably stay away from rejecting requests where they exactly match a profile. I would either match on creation and populate the field or leave creators the choice to use profiles or not as they wish.
| For workflows where replication of the same content formats are happening on a regular basis then it is recommended that the same Profile is loaded into both stores using the same UUID. | ||
| This will mean than when Flows are replicated between the stores then the Profile identifier will continue to link to the metadata. | ||
|
|
||
| If the Profile does not exist within the destination store then the Profile ID should continue to be preserved. |
There was a problem hiding this comment.
Do we need to describe how this interacts with the Profiles endpoint? Should the Profile be created if it doesn't exist? If not, how does it interact with the creation of a Profile with a matching ID? I think the paragraph below covers some of this. It might help if that is moved above this para?
There was a problem hiding this comment.
I think there is a larger issue here which I'd not spotted. To create a flow you provide either a profile or the full flow metadata. If the profile does not exist in the store then you can't actually create the flow as there is no where to get the technical metadata from. So I think you either have to fail the creation and then the calling system either needs to create the profile and then re-try and flow creation or drop the profile and just create the flow directly. The first should be the preferred option as it retains the profile information.
| It is up to the store implementation whether this is normalised on the flow creation or read-only. | ||
|
|
||
| When creating a flow using a Profile it is not possible to override any specific fields as this would invalidate the link to the Profile. | ||
| Supplying a Profile ID and technical metadata should result in a 400 validation error |
There was a problem hiding this comment.
| Supplying a Profile ID and technical metadata should result in a 400 validation error | |
| Supplying a Profile ID and technical metadata defined within the specified Profile should result in a 400 validation error |
There was a problem hiding this comment.
The line above clearly says it is not possible to override the technical parameters of a profile, so the thinking was you supply a profile or technical metadata not both. So possibly might be better saying Supplying a Profile ID and any technical metadata should result in a 400 validation error
There was a problem hiding this comment.
Sorry, yes. I had it in my head that a partial profile could be used. But I think that's a future iteration. I think your tweaked message helps. Have we explicitly defined what constitutes "technical metadata" anywhere? Specifically in the human readable version of the spec.
| "204": | ||
| description: No content. The Flow has been updated. | ||
| "400": | ||
| description: Bad request. Invalid Flow JSON. |
There was a problem hiding this comment.
Do we need to add conflicting Profiles, and invalid Profile IDs to this return code?
Updated wording Co-authored-by: James Sandford <james.sandford@bbc.co.uk>
… flow-profiles
| @@ -1,10 +1,10 @@ | |||
| { | |||
| "title": "Audio Flow", | |||
| "description": "Describes an audio Flow", | |||
There was a problem hiding this comment.
When rendered, these descriptions end up as the flow_metadata description for each type at the /profiles endpoint. I think this is unintended given the specific description in the profile schema
| in: query | ||
| description: Filter on Profile format. | ||
| schema: | ||
| $ref: 'schemas/content-format.json' |
There was a problem hiding this comment.
This includes multi, though multi-profiles aren't possible
| required: true | ||
| responses: | ||
| "201": | ||
| description: The flow has been created. |
There was a problem hiding this comment.
| description: The flow has been created. | |
| description: The Profile has been created. |
| "type": "string", | ||
| "pattern": "^[^\\s\/]+/[^\\s\/]+$" | ||
| }, | ||
| "avg_bit_rate": |
There was a problem hiding this comment.
I think avg_bit_rate and max_bit_rate either need re-locating such that they aren't in the profile, or we need to add notes to their specific CRUD endpoints stating they can't be modified where profile is set. iirc, they are update-able because they can't always be known when the Flow ingest is configured (e.g. when configuring a push-based live ingest sink) and they may vary over time in some cases.
| "type": "string", | ||
| "pattern": "^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$" |
There was a problem hiding this comment.
| "type": "string", | |
| "pattern": "^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$" | |
| "$ref": "uuid.json" |
| "type": "string", | ||
| "pattern": "^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$" |
There was a problem hiding this comment.
| "type": "string", | |
| "pattern": "^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$" | |
| "$ref": "uuid.json" |
| "type": "string", | ||
| "pattern": "^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$" |
There was a problem hiding this comment.
| "type": "string", | |
| "pattern": "^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$" | |
| "$ref": "uuid.json" |
| Further guidance on when Flows/Sources may be considered the same/different may be found in the [Practical Guidance for Media](https://specs.amwa.tv/ms-04/releases/v1.0.0/docs/3.0._Practical_Guidance_for_Media.html) section of AMWA MS-04. | ||
|
|
||
| Flows can be created either by directly supplying all the required technical metadata or by referencing an existing Profile in the store. | ||
| When using a Profile, the store will use the technical details (format, codec, essence_parameters etc.) from that profile, along with the metadata provided. |
There was a problem hiding this comment.
| When using a Profile, the store will use the technical details (format, codec, essence_parameters etc.) from that profile, along with the metadata provided. | |
| When using a Profile, the Service Implementation will populate the Flow's technical parameters (format, codec, essence_parameters etc.) from that profile, along with the metadata provided. |
|
|
||
| Flows can be created either by directly supplying all the required technical metadata or by referencing an existing Profile in the store. | ||
| When using a Profile, the store will use the technical details (format, codec, essence_parameters etc.) from that profile, along with the metadata provided. | ||
| It is up to the store implementation whether this is normalised on the flow creation or read-only. |
There was a problem hiding this comment.
| It is up to the store implementation whether this is normalised on the flow creation or read-only. | |
| It is up to the Service Implementation whether this is normalised on the Flow creation or on read. |
Tweaks based on convention for referring to TAMS entities. Slight tweak to read-only due to that being a specific parameter.
Details
Add the content of flow profiles to aid with the creation of flows in stores where there are house formats being used. Also significantly simplifies matching flows across edit by reference workflows
Jira Issue (if relevant)
Jira URL:
Related PRs
If merged before #115, add the new endpoints to the auth logic listings in that PR. If merged after, add that logic in this PR
Submitter PR Checks
(tick as appropriate)
Reviewer PR Checks
(tick as appropriate)
Info on PRs
The checks above are guidelines. They don't all have to be ticked, but they should all have been considered.