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

[patch] Use ImportAlarm from pyiron_snippets #1455

Merged
merged 6 commits into from
Jun 3, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
1 change: 1 addition & 0 deletions .ci_support/environment-docs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ dependencies:
- pint =0.23
- psutil =5.9.8
- pyfileindex =0.0.24
- pyiron_snippets =0.1.0
- pympipool =0.8.3
- pysqa =0.1.20
- pytables =3.9.2
Expand Down
1 change: 1 addition & 0 deletions .ci_support/environment-old.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ dependencies:
- pint =0.18
- psutil =5.8.0
- pyfileindex =0.0.16
- pyiron_snippets =0.1.0
- pympipool =0.8.0
- pysqa =0.1.12
- pytables =3.6.1
Expand Down
1 change: 1 addition & 0 deletions .ci_support/environment.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ dependencies:
- pint =0.23
- psutil =5.9.8
- pyfileindex =0.0.24
- pyiron_snippets =0.1.0
- pympipool =0.8.3
- pysqa =0.1.20
- pytables =3.9.2
Expand Down
1 change: 0 additions & 1 deletion .github/workflows/unittests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ on:
push:
branches: [ main ]
pull_request:
branches: [ main ]
Copy link
Member

Choose a reason for hiding this comment

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

Why was this removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I wanted to make sure this PR passes unittests. With the branch specified like that, unit tests don't run on stacked PRs.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's in the commit message: Run unittests on stacked PRs too


jobs:
build:
Expand Down
4 changes: 3 additions & 1 deletion pyiron_base/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
from pyiron_base.storage.inputlist import InputList
from pyiron_base.storage.parameters import GenericParameters
from pyiron_base.utils.deprecate import Deprecator, deprecate, deprecate_soon
from pyiron_base.utils.error import ImportAlarm
from pyiron_base.jobs.job.extension.executable import Executable
from pyiron_base.project.external import Notebook, load, dump
from pyiron_base.jobs.dynamic import warn_dynamic_job_classes
Expand Down Expand Up @@ -46,6 +45,9 @@

from pyiron_base.jobs.job.toolkit import Toolkit, BaseTools

# Expose snippets references in base API for backwards compatibility
from pyiron_snippets.import_alarm import ImportAlarm
Copy link
Member

Choose a reason for hiding this comment

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

I find this confusing - should not we do the major change and just enforce the switch to using the ImportAlarm only from pyiron_snippets?

Copy link
Member Author

Choose a reason for hiding this comment

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

I propose to leave this PR with the slightly-confusing-but-backwards-compatible redirection here, but I stacked an additional, compatibility breaking patch in #1458.

I agree that this might be slightly confusing, but I don't think it's explicitly wrong. So what I'd like to do is get go through and approve each part of the stack individually, then we can merge the whole thing in at once. The mild confusingness will thus still exist at a particular commit, but not be included in any releases.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've converted this PR to a draft until we settle on a solution for the compatibility.


# Internal init
from ._version import get_versions
from pyiron_base.utils.jedi import fix_ipython_autocomplete
Expand Down
2 changes: 1 addition & 1 deletion pyiron_base/database/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

from urllib.parse import quote_plus
from pyiron_base.state.logger import logger
from pyiron_base.interfaces.singleton import Singleton
from pyiron_snippets.singleton import Singleton
from pyiron_base.state.settings import settings as s
import os

Expand Down
37 changes: 0 additions & 37 deletions pyiron_base/interfaces/singleton.py

This file was deleted.

2 changes: 1 addition & 1 deletion pyiron_base/jobs/job/jobtype.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import os
from pyiron_base.jobs.job.util import _get_safe_job_name
from pyiron_base.storage.hdfio import ProjectHDFio
from pyiron_base.interfaces.singleton import Singleton
from pyiron_snippets.singleton import Singleton
from pyiron_base.interfaces.factory import PyironFactory
from pyiron_base.jobs.job.extension.jobstatus import job_status_finished_lst
from typing import Union
Expand Down
2 changes: 1 addition & 1 deletion pyiron_base/state/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
from pyiron_base.state.publications import publications as _publications
from pyiron_base.state.queue_adapter import queue_adapters as _queue_adapters
from pyiron_base.state.settings import settings as _settings
from pyiron_base.interfaces.singleton import Singleton
from pyiron_snippets.singleton import Singleton
from typing import Dict, Union

__author__ = "Liam Huber"
Expand Down
2 changes: 1 addition & 1 deletion pyiron_base/state/publications.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"""

import pandas
from pyiron_base.interfaces.singleton import Singleton
from pyiron_snippets.singleton import Singleton
from typing import Dict, Union, List
from typing_extensions import Literal

Expand Down
2 changes: 1 addition & 1 deletion pyiron_base/state/queue_adapter.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
multiple adapters.
"""

from pyiron_base.interfaces.singleton import Singleton
from pyiron_snippets.singleton import Singleton
from pysqa import QueueAdapter as PySQAAdpter
import os
from pyiron_base.state.settings import settings
Expand Down
2 changes: 1 addition & 1 deletion pyiron_base/state/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
from pyiron_base.state.publications import publications
from pyiron_base.utils.strtobool import strtobool
from pathlib import Path
from pyiron_base.interfaces.singleton import Singleton
from pyiron_snippets.singleton import Singleton
from typing import Union, Dict, List
from copy import deepcopy

Expand Down
2 changes: 1 addition & 1 deletion pyiron_base/storage/filedata.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import pandas

from pyiron_base.storage.hdfio import FileHDFio, ProjectHDFio
from pyiron_base.utils.error import ImportAlarm
from pyiron_snippets.import_alarm import ImportAlarm

__author__ = "Niklas Siemer"
__copyright__ = (
Expand Down
80 changes: 0 additions & 80 deletions pyiron_base/utils/error.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,9 @@
In order to be accessible from anywhere in pyiron, they *must* remain free of any imports from pyiron!
"""

import functools
from itertools import count
import time
from typing import Callable, TypeVar, Type, Tuple, Optional, Union
import warnings

from pyiron_base.state.logger import logger

Expand All @@ -27,84 +25,6 @@
__date__ = "Sep 1, 2017"


class ImportAlarm:
"""
In many places we have try/except loops around imports. This class is meant to accompany that code so that users
get an early warning when they instantiate a job that won't work when run.

Example:

>>> try:
... from mystery_package import Enigma, Puzzle, Conundrum
... import_alarm = ImportAlarm()
>>> except ImportError:
>>> import_alarm = ImportAlarm(
... "MysteryJob relies on mystery_package, but this was unavailable. Please ensure your python environment "
... "has access to mystery_package, e.g. with `conda install -c conda-forge mystery_package`"
... )
...
>>> class MysteryJob(GenericJob):
... @import_alarm
... def __init__(self, project, job_name)
... super().__init__()
... self.riddles = [Enigma(), Puzzle(), Conundrum()]

This class is also a context manager that can be used as a short-cut, like this:

>>> with ImportAlarm("MysteryJob relies on mystery_package, but this was unavailable.") as import_alarm:
... import mystery_package

If you do not use `import_alarm` as a decorator, but only to get a consistent warning message, call
:meth:`.warn_if_failed()` after the with statement.

>>> import_alarm.warn_if_failed()
"""

def __init__(self, message=None):
"""
Initialize message value.

Args:
message (str): What to say alongside your ImportError when the decorated function is called. (Default is
None, which says nothing and raises no error.)
"""
self.message = message

def __call__(self, func):
return self.wrapper(func)

def wrapper(self, function):
@functools.wraps(function)
def decorator(*args, **kwargs):
self.warn_if_failed()
return function(*args, **kwargs)

return decorator

def warn_if_failed(self):
"""
Print warning message if import has failed. In case you are not using `ImportAlarm` as a decorator you can call
this method manually to trigger the warning.
"""
if self.message is not None:
warnings.warn(self.message, category=ImportWarning)

def __enter__(self):
return self

def __exit__(self, exc_type, exc_value, traceback):
if exc_type is None and exc_value is None and traceback is None:
# import successful, so silence our warning
self.message = None
return
if issubclass(exc_type, ImportError):
# import broken; retain message, but suppress error
return True
else:
# unrelated error during import, re-raise
return False


T = TypeVar("T")


Expand Down
1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ dependencies = [
"pint==0.23",
"psutil==5.9.8",
"pyfileindex==0.0.24",
"pyiron_snippets==0.1.0",
"pympipool==0.8.3",
"pysqa==0.1.20",
"sqlalchemy==2.0.30",
Expand Down
19 changes: 0 additions & 19 deletions tests/generic/test_singleton.py

This file was deleted.

67 changes: 0 additions & 67 deletions tests/generic/test_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import warnings
from pyiron_base.utils.instance import static_isinstance
from pyiron_base.utils.deprecate import deprecate, deprecate_soon
from pyiron_base.utils.error import ImportAlarm
from pyiron_base._tests import PyironTestCase


Expand Down Expand Up @@ -145,71 +144,5 @@ def food(bar=None, baz=None):
self.assertEqual(len(w), 2, "Not all warnings preserved.")


class TestImportAlarm(PyironTestCase):
def setUp(self):
super().setUp()
self.import_alarm = ImportAlarm()

@self.import_alarm
def add_one(x):
return x + 1

with ImportAlarm("Broken import") as alarm_broken:
import ASDF

@alarm_broken
def add_two(x):
return x + 2

with ImportAlarm("Working import") as alarm_working:
import sys

@alarm_working
def add_three(x):
return x + 3

self.add_one = add_one
self.add_two = add_two
self.add_three = add_three

def test_no_warning(self):
with warnings.catch_warnings(record=True) as w:
self.add_one(0)
self.assertEqual(
len(w), 0, "Expected no warnings, but got {} warnings.".format(len(w))
)

def test_has_warning(self):
self.import_alarm.message = "Now add_one should throw an ImportWarning"

with warnings.catch_warnings(record=True) as w:
self.add_one(1)
self.assertEqual(
len(w), 1, "Expected one warning, but got {} warnings.".format(len(w))
)

def test_context(self):
"""
Usage via context manager should give same results and not suppress other errors.
"""

with warnings.catch_warnings(record=True) as w:
self.add_two(0)
self.assertEqual(
len(w), 1, "Expected one warning, but got {} warnings.".format(len(w))
)

with warnings.catch_warnings(record=True) as w:
self.add_three(0)
self.assertEqual(
len(w), 0, "Expected one warning, but got {} warnings.".format(len(w))
)

with self.assertRaises(
ZeroDivisionError, msg="Context manager should swallow unrelated exceptions"
), ImportAlarm("Unrelated"):
print(1 / 0)


if __name__ == "__main__":
unittest.main()
Loading