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.

Sign up for GitHub

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

Jump to bottom

new IR with dependency resolving #33

Merged
merged 39 commits into from
Nov 12, 2024
Merged

new IR with dependency resolving #33

merged 39 commits into from
Nov 12, 2024

Conversation

leclairm
Copy link
Contributor

@leclairm leclairm commented Nov 4, 2024

No description provided.

@leclairm leclairm self-assigned this Nov 4, 2024
@leclairm leclairm marked this pull request as ready for review November 4, 2024 21:40
agoscinski
agoscinski reviewed Nov 7, 2024
View reviewed changes
Copy link
Collaborator

@agoscinski agoscinski left a comment

Choose a reason for hiding this comment

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

Hello @leclairm, I had a first go, but need another look to have a better understanding of the construction of the workflow object. But I think it is mostly good. My main concerns is how the formatting logic is embedded, and the Store class.

src/sirocco/core.py Outdated Show resolved Hide resolved
src/sirocco/core.py Outdated Show resolved Hide resolved
src/sirocco/core.py Outdated Show resolved Hide resolved
src/sirocco/core.py Show resolved Hide resolved
src/sirocco/core.py Outdated Show resolved Hide resolved
src/sirocco/core.py Show resolved Hide resolved
src/sirocco/parsing/_yaml_data_models.py Outdated Show resolved Hide resolved
src/sirocco/parsing/_yaml_data_models.py Outdated Show resolved Hide resolved
src/sirocco/core.py Show resolved Hide resolved
DropD
DropD reviewed Nov 7, 2024
View reviewed changes
Copy link
Collaborator

@DropD DropD left a comment

Choose a reason for hiding this comment

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

Haven't looked at everything yet but here are some hopefully useful comments in the meantime!

src/sirocco/core.py Show resolved Hide resolved
src/sirocco/core.py Outdated Show resolved Hide resolved
src/sirocco/core.py Outdated Show resolved Hide resolved
src/sirocco/core.py Show resolved Hide resolved
src/sirocco/core.py Outdated Show resolved Hide resolved
src/sirocco/core.py Show resolved Hide resolved
DropD
DropD reviewed Nov 8, 2024
View reviewed changes
src/sirocco/core.py Outdated Show resolved Hide resolved
src/sirocco/core.py Outdated Show resolved Hide resolved
src/sirocco/core.py Show resolved Hide resolved
src/sirocco/core.py Show resolved Hide resolved
src/sirocco/core.py Show resolved Hide resolved
src/sirocco/parsing/_yaml_data_models.py Show resolved Hide resolved
tests/test_wc_workflow.py Show resolved Hide resolved
@leclairm leclairm mentioned this pull request Nov 11, 2024
Closed
@leclairm
Copy link
Contributor Author

fixes #30

This was referenced Nov 11, 2024
Closed
Closed
@leclairm leclairm linked an issue Nov 11, 2024 that may be closed by this pull request
arg_option should be a property of the task #30
Closed
@leclairm leclairm removed the request for review from GeigerJ2 November 11, 2024 14:22
@leclairm
fix: hatch fmt
07d45ef
@leclairm
ref: string handling
458aaa5
@leclairm
fix: check if TImeSeries initialized before testing dates
f8ba480
@leclairm
fix: revert of test order again.
ced1dbd 
the other way raises the KeyError we want to avoid when date is out of bounds
@leclairm
ref: use dataclass for Task as well
384a95b 
replace custom __init__ by from_config classmethod
@leclairm
hatch fmt
444df90
@leclairm
ref: move resolve_dates from parsing to core
656c455
@leclairm
fix: remove also from parsing ...
8052088
@leclairm
ref: move cycle date iterator from parsing to core
dc828f7
@leclairm
fix: hatch fmt
5006bc0
@leclairm
ref: put underscore back even if ruff complains
988c873
@leclairm
add: test for serialized workflow IR graph
5f8a034 
It doesn't test the non graph part, so data and task descriptions at the
bottom of the yaml file
@leclairm
ref: not only atmospheric science!
126426c
@leclairm
add:test: test against serialized data
1e7dae7
@leclairm
fix: Climate Science classifier does not exist
48c77f5 
It's a pitty!
@leclairm
ref: remove _ from DataBaseModel, used outside
7c66992
@leclairm
fix: hatch fmt
efa4206
@leclairm
ref: _resolve_target_dates is a staticmethod
2f1c751
@leclairm
fix:test: invalid suffix
3f7f677
@leclairm
ref: move testing data to its own directory
dd646b4
@leclairm
fix: generate test data in the right directory
fa4d287
@leclairm
fix: hatch fmt
8299b68
@leclairm
fix: get name clashing with dict method + typo
f431665
@leclairm
fix: hatch fmt
c82f0b5
@leclairm leclairm merged commit 354ebb5 into main Nov 12, 2024
3 checks passed
@leclairm leclairm deleted the IR_graph branch November 14, 2024 14:09
@leclairm leclairm linked an issue Nov 27, 2024 that may be closed by this pull request
New graph internal representation + visualization #23
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agoscinski agoscinski agoscinski left review comments

@DropD DropD DropD approved these changes

Assignees

@leclairm leclairm

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

arg_option should be a property of the task New graph internal representation + visualization
3 participants
@leclairm @DropD @agoscinski