Skip to content
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

Jump to bottom

Implementation of GLog #718

Merged
merged 8 commits into from
Feb 1, 2022
Merged

Implementation of GLog #718

merged 8 commits into from
Feb 1, 2022

Conversation

vlad-perevezentsev
Copy link
Collaborator

@vlad-perevezentsev vlad-perevezentsev commented Dec 9, 2021 •
edited
Loading

This PR contains the primary implementation of using GLog in dpctl.

The implementation used Glog 5.0
conda install glog

Added the --glog flag for setup in development mode
python setup.py develop --glog=True

Implemented 2 functions for initialization and deinitialization of GLog
void DPCTLService_InitLogger and void DPCTLService_ShutdownLogger

@vlad-perevezentsev vlad-perevezentsev self-assigned this Dec 9, 2021
@vlad-perevezentsev vlad-perevezentsev force-pushed the use_glog branch 2 times, most recently from ae328c7 to 11371db Compare December 9, 2021 14:51
@diptorupd
Copy link
Contributor

@vlad-perevezentsev @oleksandr-pavlyk

IMO glog usage in dpctl is a debugging feature intended for developers of dpctl or library writers who want to investigate some problem by turning on logging.

I propose the following design:

  1. libsyclinterface provides two helper functions: dpctl_initialize_glog and dpctl_shutdown_glog. These functions should be in the main library and not the helper and be exposed to Python.
  2. Change error_handler to behave one way if glog was initialized and another way if not. The function IsGoogleLoggingInitialized can be used for this purpose.
  3. So, to use advanced logging a user needs to initialize glog and the shut it down explicitly.

Few unknowns:

  1. I am not sure if glog can be shutdown and reinitialized.
  2. Can all glog-specific options be set via command line?

@vlad-perevezentsev
Copy link
Collaborator Author

@diptorupd Glog has little information in the documentation so I can't answer using it.

  1. I think Glog can be reinitiated after it is shutdown . But it doesn't continue writing to the created logfile. Glog will create a new logfile after initialization.
  2. Parameters with FLAG_ prefix can be passed via the command line.

@oleksandr-pavlyk
Copy link
Collaborator

  1. I think Glog can be reinitiated after it is shutdown . But it doesn't continue writing to the created logfile. Glog will create a new logfile after initialization.

Another important aspect is whether IsGoogleLoggingInitialized output is toggled with initialization/deinitialization. If it does, we can use it in error_handler function. User could then locally enable logging (in Python scripts with a context manager, in Google Tests with calling init/deinit functions).

@coveralls
Copy link
Collaborator

coveralls commented Dec 13, 2021 •
edited
Loading

Coverage Status

Coverage increased (+0.4%) to 81.629% when pulling 84e7dc5 on vlad-perevezentsev:use_glog into 05a5abd on IntelPython:master.

@vlad-perevezentsev vlad-perevezentsev marked this pull request as ready for review December 17, 2021 23:50
@vlad-perevezentsev vlad-perevezentsev force-pushed the use_glog branch 2 times, most recently from 8daf2e3 to a1ced1a Compare January 26, 2022 12:00
@oleksandr-pavlyk
Copy link
Collaborator

Some of the changes in this PR will be obsolete due to #746. But we should merge this once feedback is addressed, and I will update #746 after rebasing.

@oleksandr-pavlyk
Copy link
Collaborator

Note for posterity: glog=0.5 is required. The 0.4 does not declare google::EnableLogCleaner.

diptorupd
diptorupd reviewed Jan 28, 2022
View reviewed changes
CONTRIBUTING.md Outdated Show resolved Hide resolved
diptorupd
diptorupd reviewed Jan 28, 2022
View reviewed changes
CONTRIBUTING.md Outdated Show resolved Hide resolved
diptorupd
diptorupd reviewed Jan 28, 2022
View reviewed changes
CONTRIBUTING.md Outdated Show resolved Hide resolved
diptorupd
diptorupd reviewed Jan 28, 2022
View reviewed changes
CONTRIBUTING.md Outdated Show resolved Hide resolved
@oleksandr-pavlyk oleksandr-pavlyk force-pushed the use_glog branch 2 times, most recently from 62cb7e3 to 94ffffd Compare January 31, 2022 20:23
diptorupd
diptorupd reviewed Jan 31, 2022
View reviewed changes
dpctl/_dev.pyx Outdated Show resolved Hide resolved
diptorupd
diptorupd reviewed Jan 31, 2022
View reviewed changes
dpctl/_dev.pyx Outdated Show resolved Hide resolved
vlad-perevezentsev and others added 8 commits January 31, 2022 19:03
@vlad-perevezentsev @oleksandr-pavlyk
Implement InitLogger and ShutdownLogger funcs for glog and change err…
9bc62ab 
…or_handlers
@vlad-perevezentsev @oleksandr-pavlyk
Provide DPCTL_ENABLE_GLOG cmake option to turn on logging
593af45
@vlad-perevezentsev @oleksandr-pavlyk
Add glog function argument for build_backend class
73951d8
@oleksandr-pavlyk
scripts/build_locally.py supports --glog
3fd122b
@oleksandr-pavlyk
Fixes for DPCTLService_InitLogger
971d20a 
Replaced use of `stat` function with use of `filesystem::is_directory`.
Stat was always erroring out even for path to existing directory.

Created `dpctl._dev` module for developers. It adds

```python
dpctl._dev.init_logger(log_dir=None)
```

Function to initialize the logger. It uses `"dpctl"` for application name,
and supplied log directory. `log_dir=None` is interpreted as use the current
working directory.

```pyton
dpctl._dev.fini_logger()
```

Call ShutdownLogger.

```python
dpctl._dev.verbose(verbostiy="warning", log_dir=None)
```

Context manager which initializes the logger, and sets the verbosity level
for a given block of Python code:

Example:

```
In [1]: import dpctl, dpctl._dev as dd

In [2]: with dd.verbose(): dpctl.SyclDevice().parent_device
No parent for device because it is not a subdevice -33 (CL_INVALID_DEVICE) in DPCTLDevice_GetParentDevice at /localdisk/work/opavlyk/repos/dpctl/libsyclinterface/source/dpctl_sycl_device_interface.cpp:540

In [3]: dpctl.SyclDevice().parent_device  # no message

In [4]: with dd.verbose(): dpctl.SyclDevice().parent_device
No parent for device because it is not a subdevice -33 (CL_INVALID_DEVICE) in DPCTLDevice_GetParentDevice at /localdisk/work/opavlyk/repos/dpctl/libsyclinterface/source/dpctl_sycl_device_interface.cpp:540

In [5]: dpctl.SyclDevice().parent_device  # no message

In [6]: quit
```

Export DPCTLService_InitLogger and DPCTLService_FiniLogger
@oleksandr-pavlyk
Add tests to dpctl._dev.verbose
2f6aaec
@oleksandr-pavlyk
Renaming to improve self-descriptiveness.
47c3e9c 
1. GLOG defaults (log_dir=None) to outputing to color-enhanced std::cerr
2. Specifying valid log_dir changes the mode to logging
3. Context manager renamed from verbose to syclinterface_diagnostic

Test updated.
@vlad-perevezentsev @oleksandr-pavlyk
Update CONTRIBUTING.md
84e7dc5
@oleksandr-pavlyk oleksandr-pavlyk merged commit a7eafae into IntelPython:master Feb 1, 2022
@vlad-perevezentsev vlad-perevezentsev deleted the use_glog branch June 20, 2023 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleksandr-pavlyk oleksandr-pavlyk oleksandr-pavlyk left review comments

@diptorupd diptorupd diptorupd left review comments

Assignees

@vlad-perevezentsev vlad-perevezentsev

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vlad-perevezentsev @diptorupd @oleksandr-pavlyk @coveralls