Addressed some pending CR feedback

bosonnn · Mar 23, 2016 · a5a1723 · a5a1723
1 parent 7c01437
commit a5a1723
Show file tree

Hide file tree

Showing 12 changed files with 41 additions and 57 deletions.
diff --git a/Source/CNTK/CNTK.cpp b/Source/CNTK/CNTK.cpp
@@ -197,7 +197,7 @@ void DoCommands(const ConfigParameters& config, const shared_ptr<MPIWrapper>& mp
     std::cerr << "CNTKCommandTrainInfo: CNTKNoMoreCommands_Total : " << fullTotalMaxEpochs << endl;
 
     // set up progress tracing for compute cluster management
-    if (progressTracing && ((mpi == nullptr) || mpi->IsMainNode()))
+    if (progressTracing && (!mpi || mpi->IsMainNode()))
     {
         ProgressTracing::SetTracingFlag();
         ProgressTracing::TraceTotalNumberOfSteps(fullTotalMaxEpochs); // enable tracing, using this as the total number of epochs
@@ -304,7 +304,7 @@ void DoCommands(const ConfigParameters& config, const shared_ptr<MPIWrapper>& mp
             ndlScript.ClearGlobal(); // clear global macros between commands
 
             // Synchronize all ranks before proceeding to next action/command
-            if (mpi != nullptr)
+            if (mpi)
                 mpi->WaitAll();
         }
     }

diff --git a/Source/Common/Include/MPIWrapper.h b/Source/Common/Include/MPIWrapper.h
@@ -50,6 +50,9 @@ static int operator||(int rc, const MpiFail &what)
     RuntimeError("%s", what.c_str());
 }
 
+class MPIWrapper;
+typedef std::shared_ptr<MPIWrapper> MPIWrapperPtr;
+
 class MPIWrapper : public std::enable_shared_from_this<MPIWrapper>
 {
     int m_myRank;
@@ -59,7 +62,7 @@ class MPIWrapper : public std::enable_shared_from_this<MPIWrapper>
     // MPI communicator that reflects the current subset selection
     MPI_Comm m_currentComm;
 
-    static std::shared_ptr<MPIWrapper> s_mpi;
+    static MPIWrapperPtr s_mpi;
 
     // MPI_Init() with delay-loading the msmpi.dll (possibly causing a failure if missing; we want to catch that)
     int MPI_Init_DL()
@@ -223,7 +226,7 @@ class MPIWrapper : public std::enable_shared_from_this<MPIWrapper>
 
 public:
 
-    static std::shared_ptr<MPIWrapper> GetInstance(bool create = false)
+    static MPIWrapperPtr GetInstance(bool create = false)
     {
         static bool initialized = false;
         if (create)

diff --git a/Source/Common/Include/ProgressTracing.h b/Source/Common/Include/ProgressTracing.h
@@ -112,5 +112,23 @@ namespace Microsoft { namespace MSR { namespace CNTK {
 
         printf("EVALERR: %.7f%%\n", err);
     }
+
+    // This prints a PROGRESS message with a percentage value of 0 to prevent timeouts on Philly
+    // when executing long running non-training operations like PreCompute, CV, Eval, and Write
+    static size_t TraceFakeProgress(size_t numIterationsBeforePrintingProgress, size_t numItersSinceLastPrintOfProgress)
+    {
+        size_t newNumItersSinceLastPrintOfProgress = numItersSinceLastPrintOfProgress;
+        if (GetTracingFlag())
+        {
+            newNumItersSinceLastPrintOfProgress++;
+            if (newNumItersSinceLastPrintOfProgress >= numIterationsBeforePrintingProgress)
+            {
+                printf("PROGRESS: %.2f%%\n", 0.0f);
+                newNumItersSinceLastPrintOfProgress = 0;
+            }
+        }
+
+        return newNumItersSinceLastPrintOfProgress;
+    }
 };
 } } }
diff --git a/Source/ComputationNetworkLib/ComputationNetworkEvaluation.cpp b/Source/ComputationNetworkLib/ComputationNetworkEvaluation.cpp
@@ -678,8 +678,7 @@ void ComputationNetwork::MarkValueNonSharableNodes()
     std::list<ComputationNodeBasePtr> allPreComputeNodes;
     for (const auto& node : nodes)
     {
-        auto pcnode = dynamic_pointer_cast<IPreComputeNode>(node);
-        if (pcnode)
+        if (node->Is<IPreComputeNode>())
             allPreComputeNodes.push_back(node);
     }
 

diff --git a/Source/SGDLib/DataReaderHelpers.h b/Source/SGDLib/DataReaderHelpers.h
@@ -31,7 +31,7 @@ namespace Microsoft { namespace MSR { namespace CNTK {
                                         bool useParallelTrain,
                                         StreamMinibatchInputs& inputMatrices,
                                         size_t& actualMBSize, 
-                                        const std::shared_ptr<MPIWrapper>& mpi)
+                                        const MPIWrapperPtr& mpi)
     {
         auto pMBLayout = net->GetMBLayoutPtr();
         // Reading consists of a sequence of Reader API calls:

diff --git a/Source/SGDLib/IDistGradAggregator.h b/Source/SGDLib/IDistGradAggregator.h
@@ -9,7 +9,7 @@ template <class ElemType>
 class IDistGradAggregator
 {
 public:
-    IDistGradAggregator(const std::shared_ptr<MPIWrapper>& mpi)
+    IDistGradAggregator(const MPIWrapperPtr& mpi)
         : m_mpi(mpi)
     {
     }
@@ -37,7 +37,7 @@ class IDistGradAggregator
     }
 
 protected:
-    std::shared_ptr<MPIWrapper> m_mpi;
+    MPIWrapperPtr m_mpi;
 };
 
 #define UsingIDistGradAggregatorMembers           \

diff --git a/Source/SGDLib/MASGD.h b/Source/SGDLib/MASGD.h
@@ -102,7 +102,7 @@ namespace Microsoft { namespace MSR { namespace CNTK {
     {
         typedef shared_ptr<ComputationNode<ElemType>> ComputationNodePtr;
      public:
-         IMASGD(const std::shared_ptr<MPIWrapper>& pMPI, size_t perfReportFreq)
+         IMASGD(const MPIWrapperPtr& pMPI, size_t perfReportFreq)
              :m_MAworkerStatus(pMPI->NumNodesInUse(), MAWorkerStatus::NOTSTARTED), 
              m_numSyncPerformed(0), 
              m_numWorkers(pMPI->NumNodesInUse()), 
@@ -286,7 +286,7 @@ namespace Microsoft { namespace MSR { namespace CNTK {
         size_t                      m_numWorkers; 
         size_t                      m_myRank;
         MASGDPerfStats              m_perfReporter;
-        std::shared_ptr<MPIWrapper> m_pMPI;
+        MPIWrapperPtr m_pMPI;
  };
 
 
@@ -299,7 +299,7 @@ namespace Microsoft { namespace MSR { namespace CNTK {
         using Base::DownCast;
 
     public:
-        BasicModelAveragingSGD(const std::shared_ptr<MPIWrapper>& pMPI, size_t reportFreq)
+        BasicModelAveragingSGD(const MPIWrapperPtr& pMPI, size_t reportFreq)
             :Base(pMPI, reportFreq)
         {}
 

diff --git a/Source/SGDLib/SGD.cpp b/Source/SGDLib/SGD.cpp
@@ -1323,16 +1323,7 @@ bool SGD<ElemType>::PreCompute(ComputationNetworkPtr net,
 
         net->ForwardProp(nodes);
 
-        if (ProgressTracing::GetTracingFlag())
-        {
-            numItersSinceLastPrintOfProgress++;
-            if (numItersSinceLastPrintOfProgress >= numIterationsBeforePrintingProgress)
-            {
-                // TODO: For now just print 0.0 instead of calculating actual progress
-                printf("PROGRESS: %.2f%%\n", 0.0f);
-                numItersSinceLastPrintOfProgress = 0;
-            }
-        }
+        numItersSinceLastPrintOfProgress = ProgressTracing::TraceFakeProgress(numIterationsBeforePrintingProgress, numItersSinceLastPrintOfProgress);
     }
 
     // finalize

diff --git a/Source/SGDLib/SGD.h b/Source/SGDLib/SGD.h
@@ -236,7 +236,7 @@ struct SGDParams : public ScriptableObjects::Object
     bool m_useAllDataForPreComputedNode;
 
     // Parallel training
-    std::shared_ptr<MPIWrapper> m_mpi;
+    MPIWrapperPtr m_mpi;
 
     ParallelizationMethod m_parallelizationMethod;
     bool m_enableDistributedMBReading;
@@ -315,7 +315,7 @@ class SGD : public SGDParams
     {
     }
 
-    void InitMPI(const std::shared_ptr<MPIWrapper>& mpi)
+    void InitMPI(const MPIWrapperPtr& mpi)
     {
         m_mpi = mpi;
 

diff --git a/Source/SGDLib/SimpleDistGradAggregator.h b/Source/SGDLib/SimpleDistGradAggregator.h
@@ -15,7 +15,7 @@ class SimpleDistGradAggregator : public IDistGradAggregator<ElemType>
     UsingIDistGradAggregatorMembers;
 
 public:
-    SimpleDistGradAggregator(const std::shared_ptr<MPIWrapper>& mpi, bool useAsyncAggregation, int syncStatsTrace)
+    SimpleDistGradAggregator(const MPIWrapperPtr& mpi, bool useAsyncAggregation, int syncStatsTrace)
         : IDistGradAggregator<ElemType>(mpi), m_useAsyncAggregation(useAsyncAggregation), m_currentEpochNumber(-1), m_bufferedGradHeader(nullptr), m_syncStatsTrace(syncStatsTrace), m_iterationCount(0)
     {
     }

diff --git a/Source/SGDLib/SimpleEvaluator.h b/Source/SGDLib/SimpleEvaluator.h
@@ -31,7 +31,7 @@ template <class ElemType>
 class SimpleEvaluator
 {
 public:
-    SimpleEvaluator(ComputationNetworkPtr net, const std::shared_ptr<MPIWrapper>& mpi, const size_t numMBsToShowResult = 100, const int traceLevel = 0, const size_t maxSamplesInRAM = SIZE_MAX,
+    SimpleEvaluator(ComputationNetworkPtr net, const MPIWrapperPtr& mpi, const size_t numMBsToShowResult = 100, const int traceLevel = 0, const size_t maxSamplesInRAM = SIZE_MAX,
                     const size_t numSubminiBatches = 1)
         : m_net(net), 
           m_numMBsToShowResult(numMBsToShowResult), 
@@ -210,16 +210,7 @@ class SimpleEvaluator
             }
 
 
-            if (ProgressTracing::GetTracingFlag())
-            {
-                numItersSinceLastPrintOfProgress++;
-                if (numItersSinceLastPrintOfProgress >= numIterationsBeforePrintingProgress)
-                {
-                    // TODO: For now just print 0.0 instead of calculating actual progress
-                    printf("PROGRESS: %.2f%%\n", 0.0f);
-                    numItersSinceLastPrintOfProgress = 0;
-                }
-            }
+            numItersSinceLastPrintOfProgress = ProgressTracing::TraceFakeProgress(numIterationsBeforePrintingProgress, numItersSinceLastPrintOfProgress);
 
             // call DataEnd to check if end of sentence is reached
             // datareader will do its necessary/specific process for sentence ending
@@ -289,7 +280,7 @@ class SimpleEvaluator
     size_t m_numMBsToShowResult;
     size_t m_maxSamplesInRAM;
     size_t m_numSubminiBatches;
-    std::shared_ptr<MPIWrapper> m_mpi;
+    MPIWrapperPtr m_mpi;
 
     shared_ptr<IDistGradAggregator<ElemType>> m_distGradAgg;
     struct DistGradHeader* m_gradHeader;

diff --git a/Source/SGDLib/SimpleOutputWriter.h b/Source/SGDLib/SimpleOutputWriter.h
@@ -131,16 +131,7 @@ class SimpleOutputWriter
 
             totalEpochSamples += actualMBSize;
 
-            if (ProgressTracing::GetTracingFlag())
-            {
-                numItersSinceLastPrintOfProgress++;
-                if (numItersSinceLastPrintOfProgress >= numIterationsBeforePrintingProgress)
-                {
-                    // TODO: For now just print 0.0 instead of calculating actual progress
-                    printf("PROGRESS: %.2f%%\n", 0.0f);
-                    numItersSinceLastPrintOfProgress = 0;
-                }
-            }
+            numItersSinceLastPrintOfProgress = ProgressTracing::TraceFakeProgress(numIterationsBeforePrintingProgress, numItersSinceLastPrintOfProgress);
 
             // call DataEnd function in dataReader to do
             // reader specific process if sentence ending is reached
@@ -292,16 +283,7 @@ class SimpleOutputWriter
 
             fprintf(stderr, "Minibatch[%lu]: ActualMBSize = %lu\n", ++numMBsRun, actualMBSize);
 
-            if (ProgressTracing::GetTracingFlag())
-            {
-                numItersSinceLastPrintOfProgress++;
-                if (numItersSinceLastPrintOfProgress >= numIterationsBeforePrintingProgress)
-                {
-                    // TODO: For now just print 0.0 instead of calculating actual progress
-                    printf("PROGRESS: %.2f%%\n", 0.0f);
-                    numItersSinceLastPrintOfProgress = 0;
-                }
-            }
+            numItersSinceLastPrintOfProgress = ProgressTracing::TraceFakeProgress(numIterationsBeforePrintingProgress, numItersSinceLastPrintOfProgress);
 
             // call DataEnd function in dataReader to do
             // reader specific process if sentence ending is reached