Skip to content
This repository was archived by the owner on Apr 27, 2026. It is now read-only.

Cancelation and scheduling improvements#2915

Merged
colombod merged 8 commits into
dotnet:mainfrom
jonsequitur:cancel-and-scheduling-improvements
Apr 24, 2023
Merged

Cancelation and scheduling improvements#2915
colombod merged 8 commits into
dotnet:mainfrom
jonsequitur:cancel-and-scheduling-improvements

Conversation

@jonsequitur

Copy link
Copy Markdown
Contributor

No description provided.

@jonsequitur
jonsequitur marked this pull request as draft April 13, 2023 20:32
@jonsequitur
jonsequitur requested a review from colombod April 13, 2023 20:32
@jonsequitur
jonsequitur force-pushed the cancel-and-scheduling-improvements branch from f56f96c to ce31f37 Compare April 17, 2023 20:01
Comment thread src/Microsoft.DotNet.Interactive/Commands/KernelCommand.cs
return _id;
}

public bool Equals(KernelCommand other)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

consider: It may be a good idea to override object.Equals as well (and operators == and !=)

return GetOrCreateId() == other.GetOrCreateId();
}

public override int GetHashCode()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So, I am looking at adding more logging and it looks like the logging for command objects currently relies on each command type overriding ToString. I was planning to override ToString on the base KernelCommand to include the token, and then update each derived ToString to call the base implementation (in addition to returning derived-specific strings)...

I would have preferred if we didn't need to touch every command type to add this logging - I have a concern that this may not scale well. For example, if we miss the call to base.ToString() when implementing ToString for some new command(s) in the future, this could make it difficult to debug some problem in already shipped bits where the info is missing...

Another alternative would be to log detailed info about the command (including token) in a separate message. Perhaps I could do that at the same time when we stamp the routing slip (which as we discussed would also be a good place to log the routing slip info).

However, not including the token in the command.ToString() would also mean that we may miss it in some places where having that info may be useful (such as when logging the command associated with an event). So, there are pros and cons...

Any thoughts on which approach may work better @jonsequitur ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess we could also introduce a new virtual ToDisplayString method to log the derived-specific info and then only override ToString in the base type. The base ToString could then call ToDisplayString and also log the command token to ensure that the token info is always logged whenever a command object is logged. One downside is that derived command implementations should remember to only override ToDisplayString (and leave ToString untouched) to get the desired logging behavior - but that doesn't seem too bad...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We could have dedicated formatters for logging.

The .NET Interactive formatter infrastructure will let us define a single formatter for all current and future implementations of KernelCommand in one place. ToString is a fallback so we shouldn't modify it for the sake of logging.

@shyamnamboodiripad shyamnamboodiripad Apr 20, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah thanks I hadn't considered that. I guess we could just log $"Run: {value.ToDisplayString("text/logging")}" inside KernelScheduler (instead of logging $"Run: {value}" as we do currently). And the registration could look something like -

Formatter.Register(
    formatter: obj =>
    {
        if (obj is KernelCommand command)
        {
            var prefix = $"{command.GetType().Name} ({command.GetOrCreateToken()})";
    
            if (obj is SubmitCode submitCode)
            {
                return $"{prefix} {submitCode.Code}";
            }
            ...
                // Handle other command types that need custom logging
            ...
        }

        if (obj is KernelEvent @event)
        {
            ...
                // Handle logging for events
            ...
        }

        return obj.ToString();
    }, 
    mimeType: "text/logging");

Does that match what you were thinking?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that looks great!

@jonsequitur
jonsequitur force-pushed the cancel-and-scheduling-improvements branch 2 times, most recently from 90cfc96 to b41c9e0 Compare April 22, 2023 00:18
@jonsequitur
jonsequitur marked this pull request as ready for review April 22, 2023 00:18
@jonsequitur
jonsequitur enabled auto-merge (squash) April 22, 2023 00:18
@jonsequitur
jonsequitur force-pushed the cancel-and-scheduling-improvements branch from b41c9e0 to 2ffd5cf Compare April 22, 2023 00:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants