BHoM_Engine: RunExtensionMethodAsync and TryRunExtensionMethodAsync added

[pawelbaran]

Issues addressed by this PR

Closes #3140

Test files

Write a simple public static async Engine method taking BHoMObject and try running it via script uploaded here.

Changelog

Additional comments

The methods added are to large extent a rewrite of TryRunExtensionMethod. However, async methods are not allowed to have out parameters, therefore I changed the method signature compared to synchronous flavour - happy to discuss if anyone has different/better ideas 👍

[alelom]

This looks good to me. Because this is a "backend" base method, could you please add an Unit test in the BHoM_Engine_Tests solution too?

It can be a simple test that runs any existing extension method available in the base BHoM_Engine asynchronously, like Geometry() for example.

This is where the test project is: https://github.com/BHoM/BHoM_Engine/tree/develop/.ci/unit-tests/Base_Engine_Tests
and this new test should sit in a new Compute folder, in a file called like the method you're testing. Just follow the example of the IsNumeric unit test, for instance.

[pawelbaran]

This looks good to me. Because this is a "backend" base method, could you please add an Unit test in the BHoM_Engine_Tests solution too?

It can be a simple test that runs any existing extension method available in the base BHoM_Engine asynchronously, like Geometry() for example.

This is where the test project is: https://github.com/BHoM/BHoM_Engine/tree/develop/.ci/unit-tests/Base_Engine_Tests and this new test should sit in a new Compute folder, in a file called like the method you're testing. Just follow the example of the IsNumeric unit test, for instance.

Thanks @alelom for having a look. I was thinking about adding a unit test before I made one up in GH, but the problem is that all Engine methods are synchronous, and it is not possible to call one via RunExtensionMethodAsync - you naturally get MethodName does not contain a definition for 'GetAwaiter' exception. So I would need to add a dummy async extension method to the engine or call RunExtensionMethodAsync inside RunExtensionMethodAsync, none of which I was not a big fan of. What do you think?

[FraserGreenroyd]

So I would need to add a dummy async extension method to the engine or call RunExtensionMethodAsync inside RunExtensionMethodAsync, none of which I was not a big fan of. What do you think?

Haven't read everything through yet, but would agree being against adding a dummy method to the engine for the sake of testing 👍

[alelom]

Thanks @alelom for having a look. I was thinking about adding a unit test before I made one up in GH, but the problem is that all Engine methods are synchronous, and it is not possible to call one via RunExtensionMethodAsync - you naturally get MethodName does not contain a definition for 'GetAwaiter' exception. So I would need to add a dummy async extension method to the engine or call RunExtensionMethodAsync inside RunExtensionMethodAsync, none of which I was not a big fan of. What do you think?

It's actually well possible in DotNet -- just wrap the synchronous method in a Task.Run() invocation. E.g.

var result = await Task.Run(() => MySyncMethod());

This usage is well justified for this use case. See this SO post for more info.

[pawelbaran]

It's actually well possible in DotNet -- just wrap the synchronous method in a Task.Run() invocation. E.g.

var result = await Task.Run(() => MySyncMethod());

Of course, if you have a chance to call Task.Run - here we find the method by name and then await the method call:

BHoM_Engine/BHoM_Engine/Compute/TryRunExtensionMethod.cs

Line 141 in 181d3c2

return new Output<bool, object> { Item1 = true, Item2 = await (func(parameters) as dynamic) };

[alelom]

It's actually well possible in DotNet -- just wrap the synchronous method in a Task.Run() invocation. E.g.

var result = await Task.Run(() => MySyncMethod());

Of course, if you have a chance to call Task.Run - here we find the method by name and then await the method call:

BHoM_Engine/BHoM_Engine/Compute/TryRunExtensionMethod.cs

Line 141 in 181d3c2

return new Output<bool, object> { Item1 = true, Item2 = await (func(parameters) as dynamic) };

Yeah sure I meant maybe you could go ahead with a dummy method like:

public static async Task<IGeometry> GeometryAsync(this IObject obj)
{
    return await Task.Run(() => obj.Geometry());
}

and use that for the invocation in the test. Or something similar

[pawelbaran]

Yeah sure I meant maybe you could go ahead with a dummy method like:

public static async Task<IGeometry> GeometryAsync(this IObject obj)
{
    return await Task.Run(() => obj.Geometry());
}

and use that for the invocation in the test. Or something similar

Yeah, but then it needs to be added to a legit engine project to be reflected in the pool parsed by RunExtenstionMethod. So we end up with the situation commented by @FraserGreenroyd here, or I would need to create a dedicated engine for this one method...

[alelom]

Yeah, but then it needs to be added to a legit engine project to be reflected in the pool parsed by RunExtenstionMethod. So we end up with the situation commented by @FraserGreenroyd here, or I would need to create a dedicated engine for this one method...

Apologies, it all came back to my mind – this is what I was thinking of having you do. However, I just remembered that I had prepared a "Test Engine" exactly for this purpose – mainly, for testing backend methods like the RunExtensionMethod. I had even added some unit tests, and they were all running fine locally. However, we had immense trouble making them run automatically via CI/CD when BHoMBot attempted to run them via NUnit. I ended up removing them after weeks of trying.

tl;dr: let's not add this unit test method. Sorry for the long back and forth, I think it was useful to record this exchange anyways.

[FraserGreenroyd]

@BHoMBot check compliance
@BHoMBot check required

[bhombot-ci]

@FraserGreenroyd to confirm, the following actions are now queued:

• check code-compliance
• check documentation-compliance
• check project-compliance
• check branch-compliance
• check dataset-compliance
• check copyright-compliance
• check code-compliance
• check documentation-compliance
• check project-compliance
• check core
• check null-handling
• check serialisation
• check versioning
• check installer

There are 51 requests in the queue ahead of you.

[bhombot-ci]

The check code-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

[bhombot-ci]

The check documentation-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

[FraserGreenroyd]

@BHoMBot this is a DevOps instruction. I am requesting neutral checks on: unit-tests

[bhombot-ci]

@FraserGreenroyd I have provided neutral checks to the checks requested. These checks will need to be run properly to obtain full results.

[alelom]

Approving based on stated procedure and our conversation above.

[FraserGreenroyd]

@BHoMBot check ready-to-merge

[bhombot-ci]

@FraserGreenroyd to confirm, the following actions are now queued:

• check ready-to-merge

There are 129 requests in the queue ahead of you.