Move ag space to common #3192

yinweisu · 2023-05-03T00:17:45Z

Issue #, if available:

Description of changes:

Move ag space to common so it could be consumed by cloud module
Also making the way to use search space in our source code more consistent now. Now all usage would be from autogluon.common.space import XXX

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

review-notebook-app · 2023-05-03T00:17:49Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

timeseries/src/autogluon/timeseries/models/presets.py

shchur · 2023-05-03T08:50:22Z

common/src/autogluon/common/__init__.py

@@ -1,6 +1,7 @@
 from .version import __version__

 from .utils.log_utils import _add_stream_handler, fix_logging_if_kaggle as __fix_logging_if_kaggle
+from .space import Space, Categorical, Real, Int, Bool


Would it be possible to avoid a breaking change by keeping the spaces available in autogluon.core for 1 more release with a deprecation warning?

For example, using something like the following code in autogluon/core/__init__.py

import warnings class DeprecatedSpacesWrapper: def __getattr__(self, attr: str) -> Any: import autogluon.common as ag if attr in ["Space", "Categorical", "Real", "Int", "Bool"]: warnings.warn( "Accessing search spaces as `autogluon.core.space` is deprecated as of v0.8 and won't be supported " "in the next release. Please use `autogluon.common.space` instead." ) return getattr(ag.space, attr) else: raise AttributeError space = DeprecatedSpacesWrapper()

Thanks for bringing it up. I think this is a good idea

+1, this is nice

I moved the warning to the init of any Space object and imported Space related object from common in core instead.

The method @shchur provided wouldn't work if user try to do things like from autogluon.core import Real, which is possible currently. After the change, the above usage would also give proper warning message

Would this result in a warning always being produced when a Space is used (e.g., as a part of the presets), even if the user didn't use the deprecated API?

Oh yea... This is tricky. Need to think about how to cover all cases

OK. Added this inside core.space.py, and this plus the wrapper @shchur provided should cover all cases and not affect import from common

# TODO: Remove this file for v1.0 as space is moved to common import warnings import sys from typing import Any this_module = sys.modules[__name__] spaces = ["Space", "Categorical", "Real", "Int", "Bool"] class DeprecatedSpaceWrapper: def __init__(self, common_space) -> None: self.comomn_space = common_space def __call__(self, *args, **kwargs) -> Any: import autogluon.common as ag warnings.warn( "Accessing search spaces through `autogluon.core` is deprecated as of v0.8 and won't be supported " f"in the next release. Please use `from autogluon.common import {self.comomn_space}` instead." ) return getattr(ag.space, self.comomn_space)(*args, **kwargs) for s in spaces: setattr(this_module, s, DeprecatedSpaceWrapper(s))

github-actions · 2023-05-04T16:57:53Z

Job PR-3192-2a68a30 is done.
Docs are uploaded to http://autogluon-staging.s3-website-us-west-2.amazonaws.com/PR-3192/2a68a30/index.html

Innixma · 2023-05-04T18:20:04Z

core/src/autogluon/core/__init__.py

+            warnings.warn(
+                "Accessing search spaces as `autogluon.core.space` is deprecated as of v0.8 and won't be supported "
+                "in the next release. Please use `autogluon.common.space` instead."
+            )


Unsure best practice, but I wonder if we should have a class/function specifically that checks __version__ and errors if __version__ >= 0.9, else warn, as some kind of standardized logic across the repo. Currently we track the deprecations in a bunch of different ways, and can miss removing code / logic that was previously deprecated.

This wouldn't be something added in this PR, but something we may want to think about (@yinweisu @tonyhoo @gradientsky )

+1. I agree we should think about a general way to handle deprecation

We introduced the deprecated decorator for methods, but I came across a few cases where it is not applicable (e.g., when an argument to a method is renamed).

A simple approach that would cover these edge cases is to always check all the occurrences of the string deprecate within the codebase as a part of the release checklist. Though, this is definitely not elegant or scalable.

Yea I saw the deprecated decorator and I'm working on some improvement on it.

core/src/autogluon/core/hpo/executors.py

core/src/autogluon/core/hpo/ray_hpo.py

core/src/autogluon/core/hpo/space_converter.py

core/src/autogluon/core/searcher/local_grid_searcher.py

github-actions · 2023-05-04T20:06:24Z

Job PR-3192-a086431 is done.
Docs are uploaded to http://autogluon-staging.s3-website-us-west-2.amazonaws.com/PR-3192/a086431/index.html

github-actions · 2023-05-05T00:54:09Z

Job PR-3192-f6b8d32 is done.
Docs are uploaded to http://autogluon-staging.s3-website-us-west-2.amazonaws.com/PR-3192/f6b8d32/index.html

shchur

LGTM on the timeseries side, a few minor suggestions only.

shchur · 2023-05-05T12:15:37Z

timeseries/tests/unittests/test_predictor.py

@@ -6,7 +6,7 @@
 import pandas as pd
 import pytest

-import autogluon.core as ag
+from autogluon.common.space import Bool, Categorical, Int, Real, Space


I think the following code would still work and require fewer changes.

import autogluon.common as ag ... ag.space.Int(1, 3) ....

I personally also like ag.space.Categorical more than Categorical since this way it's always clear that this is a search space. This also would help to avoid potential clashes, e.g., with torch.distributions.Categorical.

What you describe makes sense. We are trying to unify the usage of the search space in our code base and we think it's not a good practice to import the whole common module while all we are using are search spaces. Maybe a better alternative would be from autogluon.common import space then space.XXX. And when there are potential other search namespace, we can do from autogluon.common import space as ag_space

Thanks for the clarification, this makes sense.

core/src/autogluon/core/__init__.py

Co-authored-by: Oleksandr Shchur <oleks.shchur@gmail.com>

github-actions · 2023-05-05T21:05:58Z

Job PR-3192-4ed8edb is done.
Docs are uploaded to http://autogluon-staging.s3-website-us-west-2.amazonaws.com/PR-3192/4ed8edb/index.html

Innixma

LGTM! Had 1 comment

Innixma · 2023-05-05T22:08:23Z

common/src/autogluon/common/__init__.py

@@ -1,6 +1,7 @@
 from .version import __version__

 from .utils.log_utils import _add_stream_handler, fix_logging_if_kaggle as __fix_logging_if_kaggle
+from .space import Space, Categorical, Real, Int, Bool


Shall we remove these imports here since we want users to do from autogluon.common.space import ...?

github-actions · 2023-05-06T01:31:52Z

Job PR-3192-168ea98 is done.
Docs are uploaded to http://autogluon-staging.s3-website-us-west-2.amazonaws.com/PR-3192/168ea98/index.html

move space to common

fcf0058

yinweisu requested review from Innixma and shchur May 3, 2023 00:17

shchur reviewed May 3, 2023

View reviewed changes

Weisu Yin added 6 commits May 3, 2023 17:02

fix

395f00d

fix

ad16843

isort

dd79416

fix

ceaf6d7

fix

8435115

isort

2a68a30

deprecated space wrapper

a086431

Innixma reviewed May 4, 2023

View reviewed changes

Weisu Yin added 2 commits May 4, 2023 19:24

refactor

6f414b7

fix

9bc3679

Weisu Yin added 5 commits May 4, 2023 20:06

fix

205b074

isort

c2d3293

fix

c0046fb

fix

e7068f4

fix

f6b8d32

shchur approved these changes May 5, 2023

View reviewed changes

Weisu Yin and others added 3 commits May 5, 2023 18:08

another refactor

70daa6a

Update core/src/autogluon/core/__init__.py

c0287eb

Co-authored-by: Oleksandr Shchur <oleks.shchur@gmail.com>

isort

4ed8edb

Innixma approved these changes May 5, 2023

View reviewed changes

address comments

168ea98

yinweisu merged commit 9bf29c0 into autogluon:master May 5, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move ag space to common #3192

Move ag space to common #3192

yinweisu commented May 3, 2023 •

edited

review-notebook-app bot commented May 3, 2023

shchur May 3, 2023 •

edited

yinweisu May 3, 2023

Innixma May 3, 2023

yinweisu May 3, 2023

shchur May 4, 2023

yinweisu May 4, 2023

yinweisu May 4, 2023

github-actions bot commented May 4, 2023

Innixma May 4, 2023

Innixma May 4, 2023

yinweisu May 4, 2023

shchur May 5, 2023

yinweisu May 5, 2023

github-actions bot commented May 4, 2023

github-actions bot commented May 5, 2023

shchur left a comment

shchur May 5, 2023

yinweisu May 5, 2023

shchur May 5, 2023 •

edited

github-actions bot commented May 5, 2023

Innixma left a comment

Innixma May 5, 2023

github-actions bot commented May 6, 2023

Move ag space to common #3192

Move ag space to common #3192

Conversation

yinweisu commented May 3, 2023 • edited

review-notebook-app bot commented May 3, 2023

shchur May 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 4, 2023

github-actions bot commented May 5, 2023

shchur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shchur May 5, 2023 • edited

Choose a reason for hiding this comment

github-actions bot commented May 5, 2023

Innixma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 6, 2023

yinweisu commented May 3, 2023 •

edited

shchur May 3, 2023 •

edited

shchur May 5, 2023 •

edited