-
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
@@ -7,6 +7,6 @@ | |||
|
|||
defs = Definitions( | |||
assets=all_assets, | |||
jobs=jobs.asset_jobs, | |||
jobs=[jobs.get_ecmwf_data], |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this 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(), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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], |
There was a problem hiding this comment.
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.
No description provided.