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 test ecmwf job #7

Merged
merged 4 commits into from
Sep 8, 2023
Merged

Add test ecmwf job #7

merged 4 commits into from
Sep 8, 2023

Conversation

devsjc
Copy link
Collaborator

@devsjc devsjc commented Sep 7, 2023

No description provided.

@@ -7,6 +7,6 @@

defs = Definitions(
assets=all_assets,
jobs=jobs.asset_jobs,
jobs=[jobs.get_ecmwf_data],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know this looks weird, but locally all the other jobs still showed up in the UI? Might be worth you pulling and checking yourself. I'm not sure if it's because they're already initialised as ScheduledJobDefinitions, and as such, don't need to go in the Definitions object? But I could be wrong!

Copy link
Member

Choose a reason for hiding this comment

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

Ah okay, yeah, that would be interesting. I am fine merging it for now, as I'm off tomorrow. But I'll check next week on it, it seems like it should have to be defined here, but yeah, not really sure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it's definitely odd. The unit test checks that there are 18 jobs, not just one, and it is passing, so it does seem that they really are all defined still! But I'm certainly confused by it as well!

@devsjc devsjc requested a review from jacobbieker September 7, 2023 15:49
Copy link
Member

@jacobbieker jacobbieker left a comment

Choose a reason for hiding this comment

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

Looks good to me overall! Still a bit confused on the Dagster configurations and assets, but will look at those next week incase something is wrong.


asset_job = define_asset_job(
name=f"download_{model}_run_{r}_{'today' if delay == 0 else 'yesterday'}",
selection=AssetSelection.all(),
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs to be changed, but from the docs, I'm not really sure how? In the UI on leonardo, I was seeing all the jobs, including the ECMWF ones, and satellite one, show up in the assets to be materialized. Not sure how to select the assets based off the prefix, or if the prefix would just be icon here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Potentially you're seeing nwp and sat in the UI on leonardo because I specified in the dagster.yaml (/home/dagster/dagster.yaml) file to display Definitions from both the sat and nwp modules, since we'll only have one UI to look at on leonardo - not sure if that's what you're referring to here?

@@ -7,6 +7,6 @@

defs = Definitions(
assets=all_assets,
jobs=jobs.asset_jobs,
jobs=[jobs.get_ecmwf_data],
Copy link
Member

Choose a reason for hiding this comment

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

Ah okay, yeah, that would be interesting. I am fine merging it for now, as I'm off tomorrow. But I'll check next week on it, it seems like it should have to be defined here, but yeah, not really sure.

@devsjc devsjc merged commit 4d044dc into main Sep 8, 2023
3 of 4 checks passed
@devsjc devsjc deleted the ecmwf-job branch September 8, 2023 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants