bpo-40799: Add _pydatetime module (Python datetime impl) #20472

vstinner · 2020-05-28T00:10:15Z

Move the Python implementation of the datetime module into a new
Lib/_pydatetime.py file to optimize "import datetime".

Define datetime.all in Lib/datetime.py.
_pydatetime.py defines name as 'datetime' for pickle.
TestModule.test_divide_and_round() now imports directly
_pydatetime, since _divide_and_round() is no longer exported by the
datetime module.
test_datetime also requires a fresh _pydatetime when loading
pure_tests.

https://bugs.python.org/issue40799

vstinner · 2020-06-08T16:20:52Z

@pganssle: This change makes "import decimal" 4.31x faster (814 us +- 32 us -> 189 us +- 4 us). In https://bugs.python.org/issue40799 you discussed the idea of converting datetime into a package. I'm not convinced by this idea, but I'm open for discussion :-) What do you think of this change? Is it to merge it?

vstinner · 2020-06-25T10:31:13Z

This PR reduce names exposed in dir(datetime).

I just updated my PR to remove datetime.sys when the pure Python implementation is used.

The C implementation exposes datetime.datetime_CAPI capsule. Maybe it can be removed, but it should be done in a separated change.

Before:

C:
    dir 19 ['MAXYEAR', 'MINYEAR', '__all__', '__builtins__', '__cached__',
            '__doc__', '__file__', '__loader__', '__name__', '__package__', '__spec__',
            'date', 'datetime', 'datetime_CAPI', 'sys', 'time', 'timedelta',
            'timezone', 'tzinfo']

Pure Python:
    dir 56 ['MAXYEAR', 'MINYEAR', '_DAYNAMES', '_DAYS_BEFORE_MONTH',
            '_DAYS_IN_MONTH', '_DI100Y', '_DI400Y', '_DI4Y', '_EPOCH',
            '_IsoCalendarDate', '_MAXORDINAL', '_MONTHNAMES', '__all__',
            '__builtins__', '__cached__', '__doc__', '__file__', '__loader__',
            '__name__', '__package__', '__spec__', '_build_struct_time',
            '_check_date_fields', '_check_time_fields', '_check_tzinfo_arg',
            '_check_tzname', '_check_utc_offset', '_cmp', '_cmperror',
            '_date_class', '_days_before_month', '_days_before_year',
            '_days_in_month', '_divide_and_round', '_format_offset',
            '_format_time', '_index', '_is_leap', '_isoweek1monday', '_math',
            '_ord2ymd', '_parse_hh_mm_ss_ff', '_parse_isoformat_date',
            '_parse_isoformat_time', '_time', '_time_class', '_tzinfo_class',
            '_wrap_strftime', '_ymd2ord', 'date', 'datetime', 'sys', 'time',
            'timedelta', 'timezone', 'tzinfo']

After:

C:

    dir 18 ['MAXYEAR', 'MINYEAR', '__all__', '__builtins__', '__cached__',
            '__doc__', '__file__', '__loader__', '__name__', '__package__',
            '__spec__', 'date', 'datetime', 'datetime_CAPI', 'time',
            'timedelta', 'timezone', 'tzinfo']

Pure Python:

    dir 17 ['MAXYEAR', 'MINYEAR', '__all__', '__builtins__', '__cached__',
            '__doc__', '__file__', '__loader__', '__name__', '__package__',
            '__spec__', 'date', 'datetime', 'time', 'timedelta',
            'timezone', 'tzinfo']

datetime.__all__ is not changed by this PR, it remains a list of 8 names for C and Python implementations:

('date', 'datetime', 'time', 'timedelta', 'timezone', 'tzinfo', 'MINYEAR', 'MAXYEAR')

vstinner · 2020-06-25T10:34:04Z

@pganssle: I plan to merge this PR next Monday. It makes "import datetime" 4.3x faster by avoiding importing time, math, sys and operator modules (when the C extension is available).

This PR doesn't prevent to convert datetime.py into a package later.

Well, see https://bugs.python.org/issue40799 for the rationale ;-)

vstinner · 2020-06-25T11:18:50Z

Ah, and this PR also fix https://bugs.python.org/issue40058 bug.

pganssle · 2020-06-25T11:20:06Z

Please don't merge this next Monday without review.

vstinner · 2020-06-25T16:08:56Z

Please don't merge this next Monday without review.

I pinged you 2 weeks ago and you didn't reply. Great if you can review it :-)

pganssle · 2020-06-25T16:37:12Z

I pinged you 2 weeks ago and you didn't reply. Great if you can review it :-)

Yes, my review queue is very long and I don't have a lot of time to clear it out.

I think this is an important change and probably a good idea, but it's also a major change, one that, AFAICT, will break all automatic backporting for datetime.py until Python 3.9 is no longer supported. I don't think there's anything particularly urgent about this change, though. In fact, because it's going to break all backporting, it might make sense to do it only after the 3.9.1 release, since it will slow down the process of getting bugs fixed in Python 3.9.

Also:

This PR doesn't prevent to convert datetime.py into a package later.

It's true that it doesn't, but unless I'm mistaken, that will be another event that breaks backporting and makes it more complicated to navigate the project history. I'm not saying that these are show-stoppers, but given that datetime has had this structure for 10 years and that this change isn't even in response to any sort of any production issue from an end user, I think it would behoove us to take the time to get it right.

vstinner · 2020-06-26T12:49:08Z

will break all automatic backporting for datetime.py until Python 3.9 is no longer supported

I tried to make a dummy change on top on my PR: a cherry-pick to 3.9 of this change without the rename change failed.

I rewrote this PR differently, and I ran the same test: the cherry-pick worked this time!

Maybe it's because this PR not only rename datetime.py, it also makes multiple changes _pydatetime.py at the same time.

vstinner · 2020-06-26T13:11:54Z

Hum, it seems like depending on the number of changes in _pydatetime.py, git cherry-pick manages to detect the rename or not.

vstinner · 2020-06-26T13:14:07Z

Between datetime.py file for datetime package (Lib/datetime/init.py), I have no preference. It seems like both options solve https://bugs.python.org/issue40799.

But I have concerns about moving Lib/_strptime.py into the package. See https://bugs.python.org/issue40799. Maybe _strptime.py can be move once these concerns are answered/fixed.

pganssle · 2020-06-26T14:45:51Z

I rewrote this PR differently, and I ran the same test: the cherry-pick worked this time!

Good to know. If we can do this in a way that cherry-pick continues to work, I'd be a lot happier merging it as early in the cycle as possible.

I'm going to comment on the _strptime and folder vs. _pydatetime.py issues in the BPO ticket, so there's a centralized place for the larger design decisions.

vstinner · 2020-06-26T14:52:15Z

Good to know. If we can do this in a way that cherry-pick continues to work, I'd be a lot happier merging it as early in the cycle as possible.

Sure, me too :-)

facundobatista · 2020-08-22T21:35:52Z

I like this change, thanks for the work!!!

The conversation in python-dev ended in agreement with this change (overe having a package), so it looks we're green to go.

@vstinner one detail, though, shouldn't we keep the __name__ attribute in datetime.py?

Move the Python implementation of the datetime module into a new Lib/_pydatetime.py file to optimize "import datetime". * Define datetime.__all__ in Lib/datetime.py. * _pydatetime.py defines __name__ as 'datetime' for pickle. * TestModule.test_divide_and_round() now imports directly _pydatetime, since _divide_and_round() is no longer exported by the datetime module. * test_datetime also requires a fresh _pydatetime when loading pure_tests.

vstinner · 2020-09-04T15:01:05Z

I rebased the PR.

vstinner · 2020-09-04T15:02:27Z

@vstinner one detail, though, shouldn't we keep the name attribute in datetime.py?

I don't understand your question. Would you mind to elaborate?

_pydatetime.py contains:

__name__ = 'datetime'   # For pickling

vstinner · 2020-09-04T15:04:01Z

I'm not sure that git blame and git cherry-pick will work once the 3 commits of this PR will be squashed into a single commit.

Maybe I will need a GitHub admin to allow me to merge without squashing, only for this PR, to help Git to detect that the file is renamed.

facundobatista

Great work, thanks!

facundobatista · 2020-09-13T01:38:16Z

@vstinner one detail, though, shouldn't we keep the name attribute in datetime.py?

I don't understand your question. Would you mind to elaborate?

_pydatetime.py contains:
__name__ = 'datetime'   # For pickling

I was on the impression that it needed to be in the "main .py" file, that somehow the "from ... import *" didn't apply to __name__, as it didn't with __doc__, but it looks I was wrong.

FTR, decimal (and it's dependant python/C implementations) follow the same patter here, so I deduce it's ok.

Thanks! again!

pganssle

LGTM, except for one change to the tests. Sorry for the long delay here.

I think maybe @pablogsal has permissions to do a rebase-merge or standard merge (to preserve the ability to do backports). Ideally we'd test this on some random branch using miss-islington if that can be arranged (though I'm not sure how easy that would be).

pganssle · 2020-09-17T20:57:37Z

Lib/test/test_datetime.py

-                                                   '_datetime', '_strptime'])
+    fast_tests = import_fresh_module(TESTS,
+                                     fresh=['datetime', '_datetime', '_strptime'],
+                                     blocked=['_pydatetime'])


Can't leave comments for lines not directly affected by the PR, but I believe we now also need to add _pydatetime to the list below:

for modname in ['datetime', '_datetime', '_pydatetime', '_strptime']: sys.modules.pop(modname, None)

(At least until we understand why that was necessary in the first place...

bedevere-bot · 2020-09-17T20:59:30Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

vstinner · 2021-03-17T20:52:21Z

I lost track of this PR and it's now outdated (in conflict). I close it. A new PR should be created.

vstinner requested a review from pganssle May 28, 2020 00:10

vstinner requested a review from abalkin as a code owner May 28, 2020 00:10

the-knights-who-say-ni added the CLA signed label May 28, 2020

bedevere-bot added the awaiting core review label May 28, 2020

shihai1991 mentioned this pull request Jun 26, 2020

bpo-40058: Fix failed test_divide_and_round of test_datetime when run more than … #19213

Closed

vstinner added 3 commits September 4, 2020 16:50

Remove datetime.sys when using _pydatetime

c14d0d4

test_datetime blocks _pydatetime

c359295

facundobatista approved these changes Sep 13, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Sep 13, 2020

pganssle requested changes Sep 17, 2020

View reviewed changes

bedevere-bot removed the awaiting merge label Sep 17, 2020

bedevere-bot added the awaiting changes label Sep 17, 2020

vstinner closed this Mar 17, 2021

vstinner deleted the pydatetime branch March 17, 2021 20:52

vstinner mentioned this pull request Apr 10, 2022

Create Lib/_pydatetime.py file to optimize "import datetime" when _datetime is available #84976

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-40799: Add _pydatetime module (Python datetime impl) #20472

bpo-40799: Add _pydatetime module (Python datetime impl) #20472

vstinner commented May 28, 2020 •

edited by bedevere-bot

Loading

vstinner commented Jun 8, 2020

vstinner commented Jun 25, 2020

vstinner commented Jun 25, 2020

vstinner commented Jun 25, 2020

pganssle commented Jun 25, 2020

vstinner commented Jun 25, 2020

pganssle commented Jun 25, 2020

vstinner commented Jun 26, 2020

vstinner commented Jun 26, 2020

vstinner commented Jun 26, 2020

pganssle commented Jun 26, 2020

vstinner commented Jun 26, 2020

facundobatista commented Aug 22, 2020

vstinner commented Sep 4, 2020

vstinner commented Sep 4, 2020

vstinner commented Sep 4, 2020

facundobatista left a comment

facundobatista commented Sep 13, 2020

pganssle left a comment

pganssle Sep 17, 2020

bedevere-bot commented Sep 17, 2020

vstinner commented Mar 17, 2021

bpo-40799: Add _pydatetime module (Python datetime impl) #20472

bpo-40799: Add _pydatetime module (Python datetime impl) #20472

Conversation

vstinner commented May 28, 2020 • edited by bedevere-bot Loading

vstinner commented Jun 8, 2020

vstinner commented Jun 25, 2020

vstinner commented Jun 25, 2020

vstinner commented Jun 25, 2020

pganssle commented Jun 25, 2020

vstinner commented Jun 25, 2020

pganssle commented Jun 25, 2020

vstinner commented Jun 26, 2020

vstinner commented Jun 26, 2020

vstinner commented Jun 26, 2020

pganssle commented Jun 26, 2020

vstinner commented Jun 26, 2020

facundobatista commented Aug 22, 2020

vstinner commented Sep 4, 2020

vstinner commented Sep 4, 2020

vstinner commented Sep 4, 2020

facundobatista left a comment

Choose a reason for hiding this comment

facundobatista commented Sep 13, 2020

pganssle left a comment

Choose a reason for hiding this comment

pganssle Sep 17, 2020

Choose a reason for hiding this comment

bedevere-bot commented Sep 17, 2020

vstinner commented Mar 17, 2021

vstinner commented May 28, 2020 •

edited by bedevere-bot

Loading