Skip to content

SINGA-117 add a switch for enabling/disabling INFO-level logging #109

Open
cac2003 wants to merge 1 commit intoapache:masterfrom
cac2003:logging
Open

SINGA-117 add a switch for enabling/disabling INFO-level logging #109
cac2003 wants to merge 1 commit intoapache:masterfrom
cac2003:logging

Conversation

@cac2003
Copy link
Copy Markdown
Contributor

@cac2003 cac2003 commented Dec 22, 2015

add switch for enabling/disabling INFO-level logging

Comment thread include/singa/log.h Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe a shorter name would be better as it is used frequently (easier for typing), e.g., DEBUG, kDebug, verbose_singa.. verbose is typically used as foo(arg1, arg2, verbose).

may put the declaration in singa.h, default value is false. the main function can parse a command line argument for this variable, e.g., -verbose true/false.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will replace it with verbose_singa.

If its declaration is put in singa.h, then every src file where this var appears should include singa.h as well as the files included in singa.h. Is it a bit redundant?

Since this is a global variable and defined as external, its real declaration and initialization should be put somewhere else. Which file do you think is appropriate for it?

btw, where is the main entrance(function) for singa located, singa/main.cc or utils/tool.cc?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

singa_verbose, Verbose, verbose_singa, which one is better?

singa.h is not included/used internally. it is to be included by user code.
hence other files inside src/ do not need to include singa.h to get this
external variable, right?

the main entrance is src/main.cc

On Wed, Dec 23, 2015 at 11:02 AM, CAI QINGCHAO notifications@github.com
wrote:

In include/singa/log.h
#109 (comment):

+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing,
+* software distributed under the License is distributed on an
+* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+* KIND, either express or implied. See the License for the
+* specific language governing permissions and limitations
+* under the License.
+*
+*************************************************************/
+
+#ifndef SINGA_LOG_H_
+#define SINGA_LOG_H_
+
+namespace singa {

  • extern bool enable_info_log;

Will replace it with verbose_singa.

If its declaration is put in singa.h, then every src file where this var
appears should include singa.h as well as the files included in singa.h. Is
it a bit redundant?

Since this is a global variable and defined as external, its real
declaration and initialization should be put somewhere else. Which file do
you think is appropriate for it?

btw, where is the main entrance(function) for singa located, singa/main.cc
or utils/tool.cc?


Reply to this email directly or view it on GitHub
https://github.com/apache/incubator-singa/pull/109/files#r48319012.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. singa_verbose?
  2. I am afraid not. If this external var is defined in singa.h, then other files where this var appears needs either to include singa.h, or to have this external variable defined in its header. (The latter seems more feasible)
  3. So we can place the real definition and initialization in src/main.cc, as long as it is compiled into singa executable.

@nudles
Copy link
Copy Markdown
Member

nudles commented Dec 23, 2015

can we do it in this way?

in singa.h

namespace singa{
  external bool singa_verbose;
}

in main.cc

#include "singa.h"
...
if -verbose in argv
   singa_verbose = true 
else 
   singa_verbose = false

@nudles
Copy link
Copy Markdown
Member

nudles commented Dec 27, 2015

@cac2003 I am going to merge this pull request. before that, pls clean the commit msgs following the format http://singa.apache.org/develop/contribute-code.html.

  1. the commit msg title should be SINGA-xxx aaaaaaaaaa
  2. squash small commits together using git rebase -i commit_id
  3. rebase to the latest master

@cac2003
Copy link
Copy Markdown
Contributor Author

cac2003 commented Dec 28, 2015

how to determine the xxx part of commit msg title?

@nudles
Copy link
Copy Markdown
Member

nudles commented Dec 28, 2015

Once you create the jira ticket you will see a ticket No., which is the xxx

Sent from my iPhone

On 28 Dec 2015, at 10:06 AM, CAI QINGCHAO notifications@github.com wrote:

how to determine the xxx part of commit msg title?


Reply to this email directly or view it on GitHub.

Add a switch for enabling/disabling INFO-level logging. This switch is off by
default. One can enable it by passing -verbose option when launching singa.
@nudles
Copy link
Copy Markdown
Member

nudles commented Dec 30, 2015

Could you pls unify the format of all key-value tuples in the log?
E.g., key = value[, key = value] (or other format that is easy to read).
It would be useful (easy) to parsing the log file to get all key-value tuples.
Thanks.

@cac2003 cac2003 changed the title Logging SINGA-117 add a switch for enabling/disabling INFO-level logging Dec 31, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants