-
Notifications
You must be signed in to change notification settings - Fork 6
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
Esgf cwt process #23
Esgf cwt process #23
Conversation
245d066
to
0f41b5d
Compare
@fmigneault This branch would be ready to merge. I'll wait for your OK, because there are some things you may want to take a look at that should have been in another PR:
I think the rest should not break this branch, although it's possible I missed some things when merging. |
5cc2abd
to
203aafc
Compare
@fmigneault I would be ready to merge this, let me know if you have any questions. |
@davidcaron @dbyrns |
tests/functional/application-packages/DeployProcess_AggregateESGF.json
Outdated
Show resolved
Hide resolved
tests/functional/application-packages/DeployProcess_SubsetESGF.json
Outdated
Show resolved
Hide resolved
tests/functional/application-packages/DeployProcess_SubsetNASAESGF.json
Outdated
Show resolved
Hide resolved
tests/functional/application-packages/Execute_AggregateESGF.json
Outdated
Show resolved
Hide resolved
@@ -185,4 +185,7 @@ def includeme(config): | |||
wps_path = get_wps_path(settings) | |||
config.add_route("wps", wps_path) | |||
config.add_view(pywps_view, route_name="wps") | |||
|
|||
config.add_static_view(get_wps_output_path(config), get_wps_output_dir(config)) |
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.
this shouldn't be required
will cause problems with nginx that is currently doing it
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.
Wouldn' t nginx override this? The way I see it it doesn't cause any harm because nginx catches the route before weaver does. And adding this makes it possible to run a local weaver without the need to have nginx running.
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.
Good point David.
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.
Also, isn't this the idea for #5?
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.
If it works in both cases, great.
Actually, we could remove it altogether from nginx, because weaver.wps_output_dir
and weaver.wps_output_path
are hardcode in it.
This variant is better if workflows are functional with it.
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 didn't think about large files... wouldn't nginx be more appropriate for large files?
Also, wouldn't setting weaver.wps_output_path = "/my-new-path"
break the correct saving of outputs anyway, even in the current setup?
I think we can have both... keep the static route in weaver, remove nginx in the config/buildout of weaver, but when we deploy, configure another instance of nginx to serve static content on a specific route.
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.
The idea with weaver.wps_output_path
is to specify which weaver url to call to obtain the process results. I couldn't find a way to cleanly match nginx's, pywps's and weaver's configs together under this same setting. So currently, it just "works" because the values are all the same...
I managed to somehow patch the pywps/weaver configs with this setting, but not nginx. I was counting on #5 to just drop it.
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.
There's not only large file, there is simultaneously access... Nginx will serve multiples files at the same time without issues while we will rapidly run out of python workers all strugling to serve the files.
For the config issue why not using an environment variable both used in the nginx and weaver config files with the template setup we have?
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.
An external nginx in the compose to serve the files is an interesting idea.
We would need to make sure that add_static_view
works standalone so that anyone can still run weaver by itself (although slower file transfer).
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.
The external nginx is already serving the files using the volume wps_outputs.
so I think we got a consensus that #5 is the way to go and adding the static serve is helpful in debugging and not harmful in production. Config wise using the compose volume wps_outputs and mounting it in both docker image at a proper place should do it.
tests/functional/application-packages/Execute_WorkflowFile_To_SubsetCRIM.json
Outdated
Show resolved
Hide resolved
aims2 is up this morning. I will test the workflows requiring aims2. |
aims2 is still down, I think it's been close to 2 weeks now... |
This pull request will not be merge. |
(https://aims2.llnl.gov/wps/ is down)
The unchecked tasks will be in a different PR.