Improve DataPipelineStatus model

[akordowski]

The DataPipelineStatus class contains multiple properties to check the status, like Completed or Failed. A comment in the class indicates that is planed to add more status properties.

kernel-memory/service/Abstractions/Models/DataPipelineStatus.cs

Lines 19 to 25 in 6d87134

	/// <summary>
	/// Currently used to track deleted documents.
	/// TODO: replace with "isDeleting" and "Deleted"
	/// </summary>
	[JsonPropertyOrder(2)]
	[JsonPropertyName("empty")]
	public bool Empty { get; set; } = false;

How about instead of having a own bool property for each status, add a enum which contains all the status values, eg:

public enum DataPipelineStatusType
{
    None,
    InProgress,
    Completed,
    Failed,
    IsDeleting,
    Deleted
}

If wished I could take a look into it and provide a PR.

[akordowski]

@dluc Any thoughts on this? (Please see also the example here)

[totti240282]

There was also a PR about pipeline improvement by @marcominerva, but has been recently closed: #443

[mikedepetris]

so true

[dluc]

Could you clarify the specific issues with the current pipeline status class?

The suggested addition of InProgress and IsDeleting could be misleading unless we verify that an action is actually scheduled in the queue. Consider a situation where the document store becomes unavailable temporarily:

• Steps in the queue would end up in the poison queue and would not be retried, meaning that nothing is actually in progress.
• The status in storage would still show InProgress since KM wouldn't be able to update the document store.

This would create a discrepancy between the actual state of the pipeline and its reported status, leading to potential confusion or incorrect assumptions.

[akordowski]

TBH I don't remember where I got the IsDeleting and Deleted values from. The general idea is to replace all the bool properties with one enum property which represents the bool property names as enum values. This would make the pipeline status check much easier.

Or is there a case when two or more properties can be true at the same time? I couldn't imagine that.

[akordowski]

Ok, I see. But what speaks against to have a generic status approach for the pipeline?

A pipeline (no matter how many steps it would have) would always have the state before start (e.g. None), then if the steps a running we would have the state InProgress, then the states Failed, Completed and so on. I hope you get the idea. The user could still create own extension methods for each steps.

No we have multiple bool properties to check against (and it could get more, see below) and then we would also have custom extension methods. At least for me this is quite confusing and not consistent.

I also fund where I got the IsDeleting and Deleted values from (see the property comment):

kernel-memory/service/Abstractions/Models/DataPipelineStatus.cs

Lines 19 to 25 in ad3e241

	/// <summary>
	/// Currently used to track deleted documents.
	/// TODO: replace with "isDeleting" and "Deleted"
	/// </summary>
	[JsonPropertyOrder(2)]
	[JsonPropertyName("empty")]
	public bool Empty { get; set; } = false;

[marcominerva]

I think that at least this should be fixed (line 441):

kernel-memory/service/Abstractions/Pipeline/DataPipeline.cs

Lines 436 to 452 in ad3e241

	public DataPipelineStatus ToDataPipelineStatus()
	{
	return new DataPipelineStatus
	{
	Completed = this.Complete,
	Failed = false, // TODO
	Empty = this.Files.Count == 0,
	Index = this.Index,
	DocumentId = this.DocumentId,
	Tags = this.Tags,
	Creation = this.Creation,
	LastUpdate = this.LastUpdate,
	Steps = this.Steps,
	RemainingSteps = this.RemainingSteps,
	CompletedSteps = this.CompletedSteps,
	};
	}

At this moment, the Failed property is always set to false, causing misleading results (a bunch of customers already reported this issue to me).

[dluc]

good point - going back to my previous rationale, I think we'll drop that property because it would never be 100% accurate

[marcominerva]

@dluc So, can I make a PR to delete this property? Then we could re-add it back when needed.

[dluc]

@marcominerva if it helps with code readability 👍

you can also remove this comment TODO: replace with "isDeleting" and "Deleted"