Skip to content

Conversation

@artidoro
Copy link
Contributor

@artidoro artidoro commented Oct 29, 2018

Fixes #1404 and #1421.

In this PR, I move the command of shutting down dotnet processes to the official build yml file. This should solve the problem of tests failing but status report being positive (the error outputs were hidden by the output of shutting down the dotnet-server).

I also disable two LightGBM test on x86, as LightGBM is not supported on x86.

I disabled two Matrix Factorization tests on x86. Matrix factorization should work on x86, but the current iteration of the code does not support it. This bug is being tracked as part of #1441.

I tested manually queuing 5 official builds and they all succeeded: https://devdiv.visualstudio.com/DevDiv/_build?definitionId=8739

Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka left a comment

Choose a reason for hiding this comment

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

:shipit:

@artidoro artidoro changed the title WIP: Moves dotnet-server shutdown to official build yml file Moves dotnet-server shutdown to official build yml file Oct 29, 2018
@artidoro artidoro requested a review from sfilipi October 29, 2018 18:12
@artidoro artidoro requested a review from wschin October 29, 2018 20:52
internal sealed class SafeTrainingAndModelBuffer : IDisposable
{
[StructLayout(LayoutKind.Explicit)]
[StructLayout(LayoutKind.Sequential)]
Copy link
Member

Choose a reason for hiding this comment

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

This is not how we typically handle this on the .NET Core team.

Instead, what we typically do is define a static/stable binary interface from the C library, and P/Invoke into that. This involves doing conversion from native sized values like int into stable values - int32_t - which maps directly to System.Int32.

I suggest following the same pattern that we do elsewhere. It has treated us very well on the .NET Core team.

/cc @wschin

Copy link
Member

@wschin wschin Oct 29, 2018

Choose a reason for hiding this comment

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

@eerhardt, could you share some examples here? I guess we still need to define structure translation between C++ and C# somewhere right? If so, what's the place in your mind? What's the difference between defining in that place and here? Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

@wschin - checkout https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/interop-guidelines.md#unix-shims for the guidance we've been using on the .NET Core team. There are some guidelines that are corefx specific - like the naming guidelines (SystemNative_).

Here is an example of the passwd struct:

The definition in C++ code:

https://github.com/dotnet/corefx/blob/88a0580e61ea757db20347f114802e73375984dd/src/Native/Unix/System.Native/pal_uid.h#L11-L23

Notice that it uses static sized integers: uint32_t - it will always be an unsigned 32-bit integer.

Here is a C++ method that returns the structure:

https://github.com/dotnet/corefx/blob/a10890f4ffe0fadf090c922578ba0e606ebdd16c/src/Native/Unix/System.Native/pal_uid.c#L62-L77

And then the corresponding matching structure in C#:

https://github.com/dotnet/corefx/blob/a10890f4ffe0fadf090c922578ba0e606ebdd16c/src/Common/src/Interop/Unix/System.Native/Interop.GetPwUid.cs#L12-L23

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

I think the P/Invoke interop into libmf needs to be changed as suggested.

@artidoro
Copy link
Contributor Author

Would you suggest doing this as part of this PR, or open a new issue about that?
The previous version of the code had hard-coded offsets, this is a simple fix to that.

@eerhardt
Copy link
Member

It would probably be best for @wschin to do the work in a separate PR.

What you are doing here is taking a step backwards IMO. Instead, let's disable the tests on x86, open a bug to fix it. That will unblock this PR, and allow for us to do "the right thing" in a separate change.

@artidoro
Copy link
Contributor Author

Sounds good, I will disable the test for now, revert the changes and open a new issue.

@artidoro
Copy link
Contributor Author

I opened an issue #1441 to track the problem brought up by Eric.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@shauheen shauheen left a comment

Choose a reason for hiding this comment

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

:shipit:

@shauheen shauheen merged commit b8267dd into dotnet:master Oct 30, 2018
@artidoro artidoro deleted the offbuild2 branch January 5, 2019 00:02
@ghost ghost locked as resolved and limited conversation to collaborators Mar 27, 2022
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.

5 participants