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.

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

Protect for missing slack_sdk import #2031

Merged
merged 6 commits into from
Mar 4, 2023

Conversation

hanlint
Copy link
Contributor

@hanlint hanlint commented Mar 3, 2023

What does this PR do?

Fixes #2030 . slack_sdk is not in the base install, so the HealthChecker needs to check the import.

Before submitting

  • Have you read the contributor guidelines?
  • Is this change a documentation change or typo fix? If so, skip the rest of this checklist.
  • Was this change discussed/approved in a GitHub issue first? It is much more likely to be merged if so.
  • Did you update any related docs and document your change?
  • Did you update any related tests and add any new tests related to your change? (see testing)
  • Did you run the tests locally to make sure they pass?
  • Did you run pre-commit on your change? (see the pre-commit section of prerequisites)

@hanlint hanlint requested a review from dakinggg March 3, 2023 22:41
Copy link
Contributor

@mvpatel2000 mvpatel2000 left a comment

Choose a reason for hiding this comment

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

Can we gate pynvml the same way?

composer/callbacks/health_checker.py Outdated Show resolved Hide resolved
composer/callbacks/health_checker.py Outdated Show resolved Hide resolved
@dakinggg
Copy link
Contributor

dakinggg commented Mar 3, 2023

manual testing evidence

In [1]: import slack_sdk
---------------------------------------------------------------------------
ModuleNotFoundError                       Traceback (most recent call last)
Cell In[1], line 1
----> 1 import slack_sdk

ModuleNotFoundError: No module named 'slack_sdk'

In [2]: from composer.callbacks import HealthChecker

In [3]: hc = HealthChecker(slack_webhook_url='test')
---------------------------------------------------------------------------
ModuleNotFoundError                       Traceback (most recent call last)
File ~/github/composer/composer/callbacks/health_checker.py:75, in HealthChecker.__init__(self, threshold, sample_freq, window_size, wait, slack_webhook_url, test_mode)
     74 try:
---> 75     import slack_sdk as slack_sdk
     76 except ImportError as e:

ModuleNotFoundError: No module named 'slack_sdk'

The above exception was the direct cause of the following exception:

MissingConditionalImportError             Traceback (most recent call last)
Cell In[3], line 1
----> 1 hc = HealthChecker(slack_webhook_url='test')

File ~/github/composer/composer/callbacks/health_checker.py:77, in HealthChecker.__init__(self, threshold, sample_freq, window_size, wait, slack_webhook_url, test_mode)
     75         import slack_sdk as slack_sdk
     76     except ImportError as e:
---> 77         raise MissingConditionalImportError('health_checker', 'slack_sdk') from e
     79 self.last_sample = 0
     80 self.last_check = 0

MissingConditionalImportError: Composer was installed without health_checker support. To use health_checker related packages, with Composer, run `pip install 'mosaicml[health_checker]'` if using pip or `conda install -c conda-forge slack_sdk` if using Anaconda.

@dakinggg
Copy link
Contributor

dakinggg commented Mar 3, 2023

New round of manual checks

without pynvml or slack_sdk

In [1]: from composer.callbacks import SpeedMonitor, HealthChecker

In [2]: hc = HealthChecker()
---------------------------------------------------------------------------
ModuleNotFoundError                       Traceback (most recent call last)
File /workdisk/danielking/github/composer/composer/callbacks/health_checker.py:146, in HealthChecker._is_available()
    145 try:
--> 146     import pynvml
    147     pynvml.nvmlInit()  # type: ignore

ModuleNotFoundError: No module named 'pynvml'

During handling of the above exception, another exception occurred:

MissingConditionalImportError             Traceback (most recent call last)
Cell In[2], line 1
----> 1 hc = HealthChecker()

File /workdisk/danielking/github/composer/composer/callbacks/health_checker.py:77, in HealthChecker.__init__(self, threshold, sample_freq, window_size, wait, slack_webhook_url, test_mode)
     74 self.last_check = 0
     76 self.metrics = []
---> 77 if self._is_available():
     78     self.metrics.append(GPUUtilization(threshold))

File /workdisk/danielking/github/composer/composer/callbacks/health_checker.py:150, in HealthChecker._is_available()
    148     return True
    149 except ImportError:
--> 150     raise MissingConditionalImportError('health_checker', 'pynvml', None)
    151 except pynvml.NVMLError_LibraryNotFound:  # type: ignore
    152     logging.warning('NVML not found, disabling GPU health checking')

MissingConditionalImportError: Composer was installed without health_checker support. To use health_checker related packages, with Composer, run `pip install 'mosaicml[health_checker]'` if using pip or `pip install pynvml` if using Anaconda.

with pynvml, without slack_sdk

In [1]: from composer.callbacks import SpeedMonitor, HealthChecker

In [2]: hc = HealthChecker()

In [3]: hc._is_available()
Out[3]: True

In [4]: hc = HealthChecker(slack_webhook_url='test')
---------------------------------------------------------------------------
ModuleNotFoundError                       Traceback (most recent call last)
File /workdisk/danielking/github/composer/composer/callbacks/health_checker.py:68, in HealthChecker.__init__(self, threshold, sample_freq, window_size, wait, slack_webhook_url, test_mode)
     67 try:
---> 68     import slack_sdk
     69     del slack_sdk

ModuleNotFoundError: No module named 'slack_sdk'

The above exception was the direct cause of the following exception:

MissingConditionalImportError             Traceback (most recent call last)
Cell In[4], line 1
----> 1 hc = HealthChecker(slack_webhook_url='test')

File /workdisk/danielking/github/composer/composer/callbacks/health_checker.py:71, in HealthChecker.__init__(self, threshold, sample_freq, window_size, wait, slack_webhook_url, test_mode)
     69         del slack_sdk
     70     except ImportError as e:
---> 71         raise MissingConditionalImportError('health_checker', 'slack_sdk', None) from e
     73 self.last_sample = 0
     74 self.last_check = 0

MissingConditionalImportError: Composer was installed without health_checker support. To use health_checker related packages, with Composer, run `pip install 'mosaicml[health_checker]'` if using pip or `pip install slack_sdk` if using Anaconda.

with pynvml and slack_sdk

In [1]: from composer.callbacks import SpeedMonitor, HealthChecker

In [2]: hc = HealthChecker(slack_webhook_url='test')

In [3]: hc._is_available()
Out[3]: True

Copy link
Contributor

@dakinggg dakinggg left a comment

Choose a reason for hiding this comment

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

LGTM

Co-authored-by: Mihir Patel <mihir.v.patel7@gmail.com>
@dakinggg dakinggg merged commit a35e32e into mosaicml:dev Mar 4, 2023
bandish-shah pushed a commit that referenced this pull request Mar 5, 2023
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.

ModuleNotFound error with latest release
3 participants