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

Add support for Python 3.11 #6326

Merged
merged 9 commits into from
Dec 8, 2022
Merged

Add support for Python 3.11 #6326

merged 9 commits into from
Dec 8, 2022

Conversation

iknox-fa
Copy link
Contributor

@iknox-fa iknox-fa commented Nov 28, 2022

resolves #6147

Description

Adds support for Python 3.11 :sonic:

Checklist

@cla-bot cla-bot bot added the cla:yes label Nov 28, 2022
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

1 similar comment
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

More tests passing locally. mypy is unhappy with the hack I've taken here, where I "call" our Policy objects as a way of initializing all their dataclass properties (now that they're default_factory instead of default). I'm sure there's a better way to do this!

core/dbt/helper_types.py Outdated Show resolved Hide resolved
@jtcohen6
Copy link
Contributor

jtcohen6 commented Dec 1, 2022

Per failing unit tests, looks like this needs fixing for py311:

def mock_import(*args, **kwargs):
mock_module = mocker.Mock()
mock_module.version = "1.0.0"
import_path = args[0]
for plugin in plugins:
if plugin in import_path:
return mock_module
raise ImportError

@MichelleArk
Copy link
Contributor

Per failing unit tests, looks like this needs fixing for py311:

Took a look into this - I believe the root cause here is that mock.patch now uses importlib.import_module indirectly python 3.11 and thus uses the mocked object (in this case, mock_import) instead of the real implementation in subsequent mock.patch calls, raising an ImportError.

The change in unittest is here. A related-looking issue was opened in cpython, but closed as not planned. One (not great, fragile) way to work around this is to swap the order of execution in test_versions.py, such that the importlib.import_module patch is the last to be mocked.

@jtcohen6
Copy link
Contributor

jtcohen6 commented Dec 5, 2022

(not great, fragile)

@MichelleArk Given that this is just a method in our unit tests (quite old, probably warranting a rewrite), I don't hate the idea of a quick & expedient fix, whose fragility we can document for our future selves

@MichelleArk MichelleArk force-pushed the jerco/py311-experiment branch 2 times, most recently from d347c37 to 6e28898 Compare December 6, 2022 04:57
@jtcohen6 jtcohen6 marked this pull request as ready for review December 6, 2022 14:07
@jtcohen6 jtcohen6 requested review from a team and leahwicz as code owners December 6, 2022 14:07
@jtcohen6 jtcohen6 requested a review from Fleid December 6, 2022 14:07
@jtcohen6 jtcohen6 changed the title Get running with Python 3.11 Add support for Python 3.11 Dec 6, 2022
@jtcohen6 jtcohen6 mentioned this pull request Dec 6, 2022
6 tasks
Copy link
Contributor

@MichelleArk MichelleArk left a comment

Choose a reason for hiding this comment

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

Just one comment - otherwise LGTM! 🐍 🏃💨

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

I think I managed to fix the failure in test_logging.py for py311.

Less sure why tests/adapter/grants suddenly started failing, claiming that the env vars for test users weren't set:

Env var required but not provided: 'DBT_TEST_USER_X'

Update: This is due to a breaking change in tox==4.0.0, see #6405

@@ -128,7 +128,7 @@ def create_debug_line(self, e: BaseEvent) -> str:
log_line = f"\n\n{separator} {datetime.utcnow()} | {self.event_manager.invocation_id} {separator}\n"
ts: str = datetime.utcnow().strftime("%H:%M:%S.%f")
scrubbed_msg: str = self.scrubber(e.message()) # type: ignore
log_line += f"{self._get_color_tag()}{ts} [{e.log_level():<5}]{self._get_thread_name()} {scrubbed_msg}"
log_line += f"{self._get_color_tag()}{ts} [{e.log_level().value:<5}]{self._get_thread_name()} {scrubbed_msg}"
Copy link
Contributor

@jtcohen6 jtcohen6 Dec 7, 2022

Choose a reason for hiding this comment

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

@peterallenwebb This seems to have been an unintentional change in #6291, where the file text log looked like:

�[0m18:00:14.981418 [EventLevel.INFO] [MainThread]: Running with dbt=1.4.0-a1
�[0m18:00:14.982553 [EventLevel.DEBUG] [MainThread]: running dbt with arguments {'write_json': True, 'use_colors': True, 'printer_width': 80, 'version_check': True, 'partial_parse': True, 'static_parser': True, 'profiles_dir': '/Users/jerco/.dbt', 'send_anonymous_usage_stats': True, 'quiet': False, 'no_print': False, 'which': 'run', 'rpc_method': 'run', 'indirect_selection': 'eager'}

Instead of:

�[0m18:10:46.308814 [info ] [MainThread]: Running with dbt=1.4.0-a1
�[0m18:10:46.309953 [debug] [MainThread]: running dbt with arguments {'write_json': True, 'use_colors': True, 'printer_width': 80, 'version_check': True, 'partial_parse': True, 'static_parser': True, 'profiles_dir': '/Users/jerco/.dbt', 'send_anonymous_usage_stats': True, 'quiet': False, 'no_print': False, 'which': 'run', 'rpc_method': 'run', 'indirect_selection': 'eager'}

Caught because this logging test checks log lines for the presence of the string ["debug"] exactly.

Update: It looks like this may have been a specific behavior change in Python 3.11, related to how enums are stringified / formatted (python/cpython#94763)

Copy link
Contributor

@jtcohen6 jtcohen6 Dec 7, 2022

Choose a reason for hiding this comment

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

I noticed that e.log_level() generally returns an EventLevel enum, but sometimes it just returns a str for DynamicLevel events (added in #6174). This was causing tests to hang (because exceptions in our logging code don't bubble up). I've handled this with a kinda janky ternary for now: 9c0af3f

Copy link
Contributor

Choose a reason for hiding this comment

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

@peterallenwebb says this is fine for now, we'll open a tech debt issue for the str/enum inconsistency :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-1411] [Feature] Support Python 3.11
4 participants