Skip to content

Academy interface cleanup #3376

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Feb 7, 2020
Merged

Academy interface cleanup #3376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a5093c5
Academy interface cleanup
Feb 6, 2020
b241945
fix tests
Feb 6, 2020
c2a9278
fix destroyImmediate use in test
Feb 6, 2020
b1e16d6
use isEditor to switch between Destroy and DestroyImmediate
Feb 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 24 additions & 14 deletions com.unity.ml-agents/Runtime/Academy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,14 +147,14 @@ public bool IsCommunicatorOn
{
Application.quitting += Dispose;

LazyInitialization();
LazyInitialize();
}

/// <summary>
/// Initialize the Academy if it hasn't already been initialized.
/// This method is always safe to call; it will have no effect if the Academy is already initialized.
/// </summary>
internal void LazyInitialization()
internal void LazyInitialize()
{
if (!m_Initialized)
{
Expand All @@ -167,7 +167,7 @@ internal void LazyInitialization()
/// Enable stepping of the Academy during the FixedUpdate phase. This is done by creating a temporary
/// GameObject with a MonoBehavior that calls Academy.EnvironmentStep().
/// </summary>
public void EnableAutomaticStepping()
void EnableAutomaticStepping()
{
if (m_FixedUpdateStepper != null)
{
Expand All @@ -184,15 +184,15 @@ public void EnableAutomaticStepping()
/// Disable stepping of the Academy during the FixedUpdate phase. If this is called, the Academy must be
/// stepped manually by the user by calling Academy.EnvironmentStep().
/// </summary>
public void DisableAutomaticStepping(bool destroyImmediate = false)
void DisableAutomaticStepping()
{
if (m_FixedUpdateStepper == null)
{
return;
}

m_FixedUpdateStepper = null;
if (destroyImmediate)
if (Application.isEditor)
{
UnityEngine.Object.DestroyImmediate(m_StepperObject);
}
Expand All @@ -205,11 +205,21 @@ public void DisableAutomaticStepping(bool destroyImmediate = false)
}

/// <summary>
/// Returns whether or not the Academy is automatically stepped during the FixedUpdate phase.
/// Determines whether or not the Academy is automatically stepped during the FixedUpdate phase.
/// </summary>
public bool IsAutomaticSteppingEnabled
public bool AutomaticSteppingEnabled
{
get { return m_FixedUpdateStepper != null; }
set {
if (value)
{
EnableAutomaticStepping();
}
else
{
DisableAutomaticStepping();
Copy link
Contributor

@vincentpierre vincentpierre Feb 7, 2020

Choose a reason for hiding this comment

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

Why is DisableAutomaticStepping still public? Seems like there are now 2 ways to disable stepping

AutomaticSteppingEnabled = false;

and

DisableAutomaticStepping()

If it is to leave the option to destroy immediate or not, why do we need to be able to do both ?

Copy link
Contributor Author

@chriselion chriselion Feb 7, 2020

Choose a reason for hiding this comment

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

From the comments above:

I assume Object.Destroy() is generally preferable, but unit tests break if we do that instead of Object.DestroyImmediate()

Open to other suggestions on how to resolve this.

Copy link
Contributor

@vincentpierre vincentpierre Feb 7, 2020

Choose a reason for hiding this comment

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

What is wrong having DestroyImmediate be the only option?

Copy link
Contributor

@vincentpierre vincentpierre Feb 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Or we could have this in the test :

        [Test]
        public void TestAcademyAutostep()
        {
            var aca = Academy.Instance;
            Assert.IsTrue(aca.IsAutomaticSteppingEnabled);
            UnityEngine.TestTools.LogAssert.Expect(LogType.Error, "Destroy may not be called from edit mode! Use DestroyImmediate instead.\nAlso think twice if you really want to destroy something in edit mode. Since this will destroy objects permanently.");
            aca.DisableAutomaticStepping(false);
            Assert.IsFalse(aca.IsAutomaticSteppingEnabled);
            aca.EnableAutomaticStepping();
            Assert.IsTrue(aca.IsAutomaticSteppingEnabled);
        }

Copy link
Contributor

@surfnerd surfnerd Feb 7, 2020

Choose a reason for hiding this comment

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

DestroyImmediate is strongly recommended against since it tries to deallocate memory immediately instead of batching it together after a normal Destroy call.

Copy link
Contributor

@vincentpierre vincentpierre Feb 7, 2020

Choose a reason for hiding this comment

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

Should we make DisableAutomaticStepping internal (This way we can test it without exposing it?)

Copy link
Contributor Author

@chriselion chriselion Feb 7, 2020

Choose a reason for hiding this comment

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

DisableAutomaticStepping is now private; I use Application.isEditor to switch between Destroy and DestoryImmediate.

}
}
}

// Used to read Python-provided environment parameters
Expand Down Expand Up @@ -334,9 +344,9 @@ void OnResetCommand()
/// <returns>
/// Current episode number.
/// </returns>
public int GetEpisodeCount()
public int EpisodeCount
{
return m_EpisodeCount;
get { return m_EpisodeCount; }
}

/// <summary>
Expand All @@ -345,9 +355,9 @@ public int GetEpisodeCount()
/// <returns>
/// Current step count.
/// </returns>
public int GetStepCount()
public int StepCount
{
return m_StepCount;
get { return m_StepCount; }
}

/// <summary>
Expand All @@ -356,9 +366,9 @@ public int GetStepCount()
/// <returns>
/// Total step count.
/// </returns>
public int GetTotalStepCount()
public int TotalStepCount
{
return m_TotalStepCount;
get { return m_TotalStepCount; }
}

/// <summary>
Expand Down Expand Up @@ -444,7 +454,7 @@ internal ModelRunner GetOrCreateModelRunner(
/// </summary>
public void Dispose()
{
DisableAutomaticStepping(true);
DisableAutomaticStepping();
// Signal to listeners that the academy is being destroyed now
DestroyAction?.Invoke();

Expand Down
42 changes: 21 additions & 21 deletions com.unity.ml-agents/Tests/Editor/MLAgentsEditModeTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,9 @@ public void TestAcademy()
{
var aca = Academy.Instance;
Assert.AreNotEqual(null, aca);
Assert.AreEqual(0, aca.GetEpisodeCount());
Assert.AreEqual(0, aca.GetStepCount());
Assert.AreEqual(0, aca.GetTotalStepCount());
Assert.AreEqual(0, aca.EpisodeCount);
Assert.AreEqual(0, aca.StepCount);
Assert.AreEqual(0, aca.TotalStepCount);
}

[Test]
Expand Down Expand Up @@ -165,12 +165,12 @@ public void TestAcademy()
Assert.AreEqual(true, Academy.IsInitialized);

// Check that init is idempotent
aca.LazyInitialization();
aca.LazyInitialization();
aca.LazyInitialize();
aca.LazyInitialize();

Assert.AreEqual(0, aca.GetEpisodeCount());
Assert.AreEqual(0, aca.GetStepCount());
Assert.AreEqual(0, aca.GetTotalStepCount());
Assert.AreEqual(0, aca.EpisodeCount);
Assert.AreEqual(0, aca.StepCount);
Assert.AreEqual(0, aca.TotalStepCount);
Assert.AreNotEqual(null, aca.FloatProperties);

// Check that Dispose is idempotent
Expand Down Expand Up @@ -247,8 +247,8 @@ public void TestAcademy()
var numberReset = 0;
for (var i = 0; i < 10; i++)
{
Assert.AreEqual(numberReset, aca.GetEpisodeCount());
Assert.AreEqual(i, aca.GetStepCount());
Assert.AreEqual(numberReset, aca.EpisodeCount);
Assert.AreEqual(i, aca.StepCount);

// The reset happens at the beginning of the first step
if (i == 0)
Expand All @@ -263,11 +263,11 @@ public void TestAcademy()
public void TestAcademyAutostep()
{
var aca = Academy.Instance;
Assert.IsTrue(aca.IsAutomaticSteppingEnabled);
aca.DisableAutomaticStepping(true);
Assert.IsFalse(aca.IsAutomaticSteppingEnabled);
aca.EnableAutomaticStepping();
Assert.IsTrue(aca.IsAutomaticSteppingEnabled);
Assert.IsTrue(aca.AutomaticSteppingEnabled);
aca.AutomaticSteppingEnabled = false;
Assert.IsFalse(aca.AutomaticSteppingEnabled);
aca.AutomaticSteppingEnabled = true;
Assert.IsTrue(aca.AutomaticSteppingEnabled);
}

[Test]
Expand Down Expand Up @@ -358,9 +358,9 @@ public void TestAcademy()
var stepsSinceReset = 0;
for (var i = 0; i < 50; i++)
{
Assert.AreEqual(stepsSinceReset, aca.GetStepCount());
Assert.AreEqual(numberReset, aca.GetEpisodeCount());
Assert.AreEqual(i, aca.GetTotalStepCount());
Assert.AreEqual(stepsSinceReset, aca.StepCount);
Assert.AreEqual(numberReset, aca.EpisodeCount);
Assert.AreEqual(i, aca.TotalStepCount);
// Academy resets at the first step
if (i == 0)
{
Expand Down Expand Up @@ -396,10 +396,10 @@ public void TestAgent()
var agent2StepSinceReset = 0;
for (var i = 0; i < 5000; i++)
{
Assert.AreEqual(acaStepsSinceReset, aca.GetStepCount());
Assert.AreEqual(numberAcaReset, aca.GetEpisodeCount());
Assert.AreEqual(acaStepsSinceReset, aca.StepCount);
Assert.AreEqual(numberAcaReset, aca.EpisodeCount);

Assert.AreEqual(i, aca.GetTotalStepCount());
Assert.AreEqual(i, aca.TotalStepCount);

Assert.AreEqual(agent2StepSinceReset, agent2.GetStepCount());
Assert.AreEqual(numberAgent1Reset, agent1.agentResetCalls);
Expand Down