-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Movement and Internalization Phase 2 #1626
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d9fad39
72d811d
a5a0310
9b4810e
af9cb68
479effd
fe71454
936fbbf
4aabd31
fdb4d7c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,13 +9,15 @@ namespace Microsoft.ML.Runtime.EntryPoints | |
/// <summary> | ||
/// This is a signature for classes that are 'holders' of entry points and components. | ||
/// </summary> | ||
public delegate void SignatureEntryPointModule(); | ||
[BestFriend] | ||
internal delegate void SignatureEntryPointModule(); | ||
|
||
/// <summary> | ||
/// A simplified assembly attribute for marking EntryPoint modules. | ||
/// </summary> | ||
[AttributeUsage(AttributeTargets.Assembly, AllowMultiple = true)] | ||
public sealed class EntryPointModuleAttribute : LoadableClassAttributeBase | ||
[BestFriend] | ||
internal sealed class EntryPointModuleAttribute : LoadableClassAttributeBase | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, assuredly. Do you feel this PR needs to be longer though? I'm trying to break up the internalization work into multiple phases (hence the numbering, phase 1, phase 2, and so forth). While surely we can just keep going forever, I think it would be more helpful if we tried to iterate quickly, rather than insisting we internalize everything in one gigantic PR. So just pointing out "you could have internalized more," while I'm sure well intentioned, is something I already kind of know and understand? In reply to: 234256509 [](ancestors = 234256509) |
||
{ | ||
public EntryPointModuleAttribute(Type loaderType) | ||
: base(null, typeof(void), loaderType, null, new[] { typeof(SignatureEntryPointModule) }, loaderType.FullName) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,8 +5,6 @@ | |
#pragma warning disable 420 // volatile with Interlocked.CompareExchange | ||
|
||
using System; | ||
using System.Collections.Concurrent; | ||
using System.Collections.Generic; | ||
using System.IO; | ||
using System.Linq; | ||
using System.Threading; | ||
|
@@ -15,7 +13,12 @@ namespace Microsoft.ML.Runtime.Data | |
{ | ||
using Stopwatch = System.Diagnostics.Stopwatch; | ||
|
||
public sealed class ConsoleEnvironment : HostEnvironmentBase<ConsoleEnvironment> | ||
/// <summary> | ||
/// The console environment. As its name suggests, should be limited to those applications that deliberately want | ||
/// console functionality. | ||
/// </summary> | ||
[BestFriend] | ||
internal sealed class ConsoleEnvironment : HostEnvironmentBase<ConsoleEnvironment> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe we can close #1284 with this change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
{ | ||
public const string ComponentHistoryKey = "ComponentHistory"; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this is the right change for
AssemblyLoadingUtils
. I purposefully made it as "shared source" betweenMaml
and theML.Legacy
assemblies, with the hopes thatML.Legacy
will just go away, so this will only be used inMaml
.I don't think it should be in the Core assembly (and especially not with a
[BestFriend]
attribute). I don't want people calling this code at all.Maml
andML.Legacy
need it to keep things working, but I don't want any other places calling it. I've instructed other internal projects, if they need this code, they can copy it and modify it to meet their needs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, people have been ignoring your instructions. 😄 A couple more now use it.
I made
ICommand
and allICommand
implementors internal. Things that directly call some of the commands now had to reference them. In the case ofMaml
this wasLegacy
andResultProcessor
, both of which actually use thisAssemblyLoadingUtils
. So I madeMaml
considerLegacy
andResultProcessor
friends ofMaml
, so that they could keep using it. But of course this results in an error, because they'reHowever, because you made this a source reference, I then had assemblies referencing each other, which led to name collisions resulting in compiler errors. This was my attempt to untangle the mess. I can't just go back to the way things were because that doesn't compile. I didn't perform this move in a fit of whimsy.
It may be that eventually we can winnow it down, but for right now, I'm not sure what else is better to do.
I'm not sure what you mean about "especially not." Just because something is a best-friend internal doesn't mean we're going to keep it now and forever. Indeed the point of introducing this infrastructure in the first place is so we would be free to later whip the internal infrastructure code into a better form, without compromising v1.0. This I think falls pretty squarely into its intended use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means more places will feel empowered to call this method, because they won't get a "Don't use this" error. And as more places use it, the harder it will be to remove in the future. (Especially places that aren't in this repo, that are planning on getting IVT the Core assembly)
Can we instead do some other "tricks" to make it not part of the Core assembly? Maybe using
extern alias
? https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/extern-aliasOr maybe we have compiler directives that change the namespace of this file/class in the different assemblies it is used in?
I'd really like this type not to be in the Core assembly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I'll mark it obsolete as I did with
IHostEnvironment.GetTempFile
. That will give people a nice little message discouraging them, without requiring that I introduce special build infrastructure just for this one little file, which frankly I don't want to do.In reply to: 234269562 [](ancestors = 234269562)