Page MenuHomeElementl

Remove inclusive parameter when generating schedule partition ranges
ClosedPublic

Authored by rexledesma on May 27 2021, 5:20 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, May 22, 1:44 AM
Unknown Object (File)
Thu, May 18, 5:52 PM
Unknown Object (File)
Wed, May 17, 9:18 AM
Unknown Object (File)
Mon, May 15, 12:01 PM
Unknown Object (File)
Sun, May 14, 1:14 PM
Unknown Object (File)
Sun, May 14, 11:04 AM
Unknown Object (File)
Sun, May 14, 11:04 AM
Unknown Object (File)
Sun, May 14, 6:07 AM
Subscribers
None

Details

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ignore airflow tests failing, haven't rebased on master yet

sorry for the delay, this one took me a while to wrap my brain around.

python_modules/dagster/dagster/core/definitions/partition.py
79–96

suggestion - once you have defined 'now' avoid using 'current_time' because its super confusing to have both in active service

how about:

current_time = current_time if current_time else pendulum.now(tz) - and then no more need for now?

110

this line is kind of breaking my brain (particularly when I am trying to understand the distinction between 'current time' and 'now' which sound like they should be the same thing?).

Here's the version of this section that I think makes sense to me (and doesn't need to care about current_time) but I may be missing something:

if end: # Partition set has an explicit end time that represents the end of the partition range
   end_partition_time = _end
else: #  Partition set lasts forever - _end is the current time, last partition time should be the last partition that executes before that time
   end_partition_time = execution_time_to_partition_fn(_end)

end_timestamp = end_partition_time.timestamp()

If I'm understanding this correctly I think that would eliminate the need for the min below as well?

python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_decorators.py
487

just confirming understanding - this was incorrectly including an additional partition at the end before (since the default used to be partition_hours_offset = 1 and now there's one fewer partition in that case)?

I guess i'm a little confused how the cron scheduler worked if that was the case since it always picks the last partition

python_modules/dagster/dagster_tests/core_tests/partition_tests/test_partition.py
193

so it looks like this is including the execution time in the end date which I wouldn't expect a user to need to do - are we just testing that pathological case? But a lot of these tests seem to include that which makes me suspect something not right is happening. The intention is that you can pass in the end partition date (not the end execution date) and everything will still work

This revision now requires changes to proceed.Jun 2 2021, 3:58 PM
python_modules/dagster/dagster/core/definitions/partition.py
110

[1] I include the min statement, otherwise all the partitions will always be shown if end is specified. I think it makes sense to ignore the partitions that could not have executed yet.

python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_decorators.py
487

So this was working correctly for partition_hours_offset == 0 and == 1. When > 2, there were extra partitions.

python_modules/dagster/dagster_tests/core_tests/partition_tests/test_partition.py
193

Yeah this is just a pathological example. It should still work if I remove the hour and minute from the end partition date.

end in tests should be the end of the partition time, add additional tests to test for when end is not specified, update get_schedule_range_partitions logic to be more clear

python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_decorators.py
487

we still have this mystery right? The most likely candidate would be that it is somehow incorrectly returning one additional partition at the end where it wasn't before

remove subtract in favor of replace

sweet, thank you for powering through that, the universe makes sense again :)

This revision is now accepted and ready to land.Jun 9 2021, 10:19 PM