Skip to content

[BEAM-10018] Fix timestamps in two windowing Python katas - #11731

Merged
pabloem merged 6 commits into
apache:masterfrom
iht:window_python_katas
May 18, 2020
Merged

[BEAM-10018] Fix timestamps in two windowing Python katas#11731
pabloem merged 6 commits into
apache:masterfrom
iht:window_python_katas

Conversation

@iht

@ihtiht commented May 16, 2020

Copy link
Copy Markdown
Contributor

The two Python katas about windowing fail because the timestamps for the elements are calculated based on the local timezone, and my timezone does not match the timezone hardcoded in the tests, and because parsing from strings using fromisoformat was failing.

In the first Kata, the timestamp was calculated from time objects, and converted to a timestamp in the local timezone. Thus, the results of the test depended on the configuration of the local timezone in the running system.

The tests were hardcoded with a timezone different to mine, and thus I always failed to pass this Kata. The changes in this commit change the type in Event to be a datetime, the timestamps are set in UTC, and the output in the tests is hardcoded in UTC too. This should ensure that the kata works regardless the timezone configured in the system running the kata.

In the second Kata, the code was failing with the following error:

AttributeError: type object 'datetime.datetime' has no attribute 'fromisoformat'

I changed the timestamps to be set using the datetime constructor, rather than parsing from strings using fromisoformat.

Both katas now pass with the examples hardcoded in the corresponding tests.

Post-Commit Tests Status (on master branch)

LangSDKApexDataflowFlinkGearpumpSamzaSpark
GoBuild Status------Build Status------Build Status
JavaBuild StatusBuild StatusBuild Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build StatusBuild StatusBuild Status
Build Status
Build Status
PythonBuild Status
Build Status
Build Status
Build Status
---Build Status
Build Status
Build Status
Build Status
Build Status
------Build Status
XLang---------Build Status------Build Status

Pre-Commit Tests Status (on master branch)

---JavaPythonGoWebsite
Non-portableBuild StatusBuild Status
Build Status
Build StatusBuild Status
Portable---Build Status------

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

iht added 2 commits May 16, 2020 19:21
In this Kata, the timestamp was calculated from time objects, and converted to a
timestamp in the local timezone. Thus, the results of the test depended on the
configuration of the local timezone in the running system.
The tests were hardcoded with a timezone different to mine, and thus I always
failed to pass this Kata. The changes in this commit change the type in Event to
be a datetime, the timestamps are set in UTC, and the output in the tests is
hardcoded in UTC too. This should ensure that the kata works regardless the
timezone configured in the system running the kata.
Parsing the timestamps as strings using fromisoformat was failing, and the Kata
failed silently regardless the code written in the boxes.
This change sets the same timestamps, with UTC timezone, without parsing
strings.
@iht

iht commented May 16, 2020

Copy link
Copy Markdown
ContributorAuthor

R: @henryken

@henrykenhenryken left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for submitting the fix, Israel!
I have added some comments.

If you need further help on the answer placeholder fix, please let me know.

Comment threadlearning/katas/python/Windowing/Adding Timestamp/ParDo/task-info.yaml Outdated
Comment threadlearning/katas/python/Windowing/Adding Timestamp/ParDo/task.py Outdated
visible: true
placeholders:
- offset: 2074
- offset: 2067

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Need to fix the answer placeholder
image

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I am checking the boundaries of the placeholder in PyCharm, and I see it correctly
image
Maybe you were checking the previous placeholder box? I see that the box is starting 7 positions to the right (2074, after the Wi), and the placeholder I am submitting is starting 7 positions to the left (2067).

I have removed all the Placeholders and created them again by selecting the answer in the code, and using right click "Add answer placeholder", and the bounding boxes are the same I have committed (same values in the section placeholders in task-info.yaml). I am not sure if I am doing something wrong though. It is the first time I generate Edu content for PyCharm.

@henryken

Copy link
Copy Markdown
Contributor

Thanks for the changes @iht. LGTM now.
@pabloem, can you help to merge and close this issue?

@pabloem

Copy link
Copy Markdown
Member

retest this please

@pabloem

Copy link
Copy Markdown
Member

making sure license tests pass

@pabloem
pabloem merged commit 1f21a4c into apache:masterMay 18, 2020
@pabloem

Copy link
Copy Markdown
Member

thanks everyone!

@iht

iht commented May 18, 2020

Copy link
Copy Markdown
ContributorAuthor

Thank you!

@iht
iht deleted the window_python_katas branch May 18, 2020 19:53
Sign up for freeto 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.

3 participants

@iht@henryken@pabloem