I26 use urls #27

eyvorchuk · 2020-08-24T17:03:17Z

This PR resolves issue 26, which is to allow the config file for wps_parameters.py to have FILE_PATHs pointing to data files on THREDDS. For each URL, the data was copied to a temporary file in the working directory of the process, and the URL in the config file was replaced with the corresponding local file path. This functionality was tested by mocking the URLs prior to running the process to avoid requesting data that may not exist on THREDDS.

eyvorchuk · 2020-08-24T17:05:18Z

@jameshiebert I should mention that some of the RVIC modules in the venv need to be modified before the tests will work. The necessary changes are in the testing section of the README.

sum1lim · 2020-08-24T17:25:23Z

tests/configs/parameter_https.cfg

@@ -73,7 +73,7 @@ SEARCH_FOR_CHANNEL: False
 #-- Path to Pour Points File (char) --#
 # A comma separated file of outlets to route to [lons, lats] - one coordinate pair per line (order not important)
 # May optionally include a column [names] - which will (if not aggregating) be included in param file
-FILE_NAME: https://docker-dev03.pcic.uvic.ca/twitcher/ows/proxy/thredds/dodsC/datasets/RVIC/sample_pour.txt
+FILE_NAME: https://docker-dev03.pcic.uvic.ca/twitcher/ows/proxy/thredds/fileServer/datasets/RVIC/sample_pour.txt



I think the ideal use of pour input is to get user inputs (lon and lat) and create a pour.txt file in the tmp directory. It is a variable that needs to be dynamically defined by users. @jameshiebert @corviday , do you think this is the right approach? I guess we don't have to worry about that in this PR. I will write an issue if it is needed.

sum1lim

pytest is not passing for me. The error message is attached to one of the comments.

While it is good to download files from https, there is a method you can use opendap as I did in a thunderbird PR. xarray library enables to copy netcdf contents retrieved from oepdap URL. You can create a copy of the files with the contents in the tmp directory. I think it is worth to consider this method since I implemented thunderbird/wps_update_metadata this way.

sum1lim · 2020-08-24T17:55:03Z

tests/configs/parameter_https.cfg

@@ -73,7 +73,7 @@ SEARCH_FOR_CHANNEL: False
 #-- Path to Pour Points File (char) --#


It would be nice to use this file in the jupyter lab demo as well

sum1lim · 2020-08-24T17:56:18Z

tests/test_wps_parameters.py

+@pytest.mark.parametrize(
+    ("config"), [resource_filename(__name__, "configs/parameter_https.cfg")],
+)
+def test_parameters_https(config, make_mock_urls):


This test is failing with the following error message:

Traceback (most recent call last): File "/home/sangwonl/Desktop/osprey/venv/lib/python3.6/site-packages/pywps/app/Process.py", line 249, in _run_process self.handler(wps_request, wps_response) # the user must update the wps_response. File "/home/sangwonl/Desktop/osprey/osprey/processes/wps_parameters.py", line 119, in _handler parameters(config, np) File "/home/sangwonl/Desktop/osprey/venv/lib/python3.6/site-packages/rvic/parameters.py", line 51, in parameters outlets, config_dict, directories = gen_uh_init(config_file) File "/home/sangwonl/Desktop/osprey/venv/lib/python3.6/site-packages/rvic/parameters.py", line 161, in gen_uh_init config_dict = copy_inputs(config_file, directories['inputs']) File "/home/sangwonl/Desktop/osprey/venv/lib/python3.6/site-packages/rvic/core/utilities.py", line 243, in copy_inputs copyfile(section['FILE_NAME'], new_file_name) File "/usr/lib/python3.6/shutil.py", line 120, in copyfile with open(src, 'rb') as fsrc: FileNotFoundError: [Errno 2] No such file or directory: '/tmp/pywps_process_36j0za4t/sample_pour2b3izq8y.txt'

What does each FILE_PATH in your parameter_https.cfg file look like? I just realized that running the notebook as is permanently changes it.

Actually, the demo doesn't use that .cfg file, so it won't be modified. I'll look into it more.

Could you try replacing self.workdir in this line with a different directory, and after the for loop in replace_urls, print filedata? I just want to see if the contents of the config file are being modified correctly.

What does each FILE_PATH in your parameter_https.cfg file look like? I just realized that running the notebook as is permanently changes it.

I think you are right here. I did git stash and ran pytest again, and it passed.

Actually, the demo doesn't use that .cfg file, so it won't be modified. I'll look into it more.

Last time I ran the demo first by changing the input to the parameter_https.cfg, so that caused the issue you said.

jameshiebert · 2020-08-24T20:05:34Z

@jameshiebert I should mention that some of the RVIC modules in the venv need to be modified before the tests will work. The necessary changes are in the testing section of the README.

Could you fix this in RVIC and open a PR against RVIC as requested by Joe Hamman for your second testing note? Testing should be automated; it's not feasible/scalable to have to make manual changes to an installed virtual environment. We need to fix the root of this problem. And if its as simple as you note, it should be a no brainer.

sum1lim · 2020-08-24T20:22:37Z

@jameshiebert I already made a PR in RVIC repo and emailed Joe Hamman, but still waiting for the reply.

eyvorchuk · 2020-08-24T20:22:54Z

Could you fix this in RVIC and open a PR against RVIC as requested by Joe Hamman for your second testing note? Testing should be automated; it's not feasible/scalable to have to make manual changes to an installed virtual environment. We need to fix the root of this problem. And if its as simple as you note, it should be a no brainer.

Unfortunately, we haven't had luck with getting those issues fixed in RVIC. The first issue has been fixed in RVIC 1.1.1, but for some reason, that version isn't installable as a python package (the only one available is v1.1.0.post1). @sum1lim opened a PR for the other issue a month ago and I believe emailed one of the devs about it, but neither of those have gotten responses.

nikola-rados

Overall I think this is a nice solution to the issue, good work! That being said I am getting failures on my pytest runs locally. I have followed the instructions in the README but still get issues. Does it work locally for you? Are there any changes I need to make that aren't in the README?

nikola-rados · 2020-08-31T18:15:46Z

osprey/cli.py

@@ -152,7 +152,7 @@ def stop():
 @click.option(
    "--parallelprocesses",
    metavar="INT",
-    default="2",
+    default="20",


Revert this change

nikola-rados · 2020-08-31T18:21:24Z

osprey/utils.py

+    read_config = open(config, "r")
+    filedata = read_config.readlines()
+    read_config.close()


We could use a context manager here.

nikola-rados · 2020-08-31T18:23:32Z

osprey/utils.py

+    for i in range(len(filedata)):
+        if "https" in filedata[i]:
+            url = filedata[i].split(" ")[-1]  # https url is last word in line
+            url = url.rstrip()  # remove \n character at end
+            r = requests.get(url)
+            filename = url.split("/")[-1]
+            prefix, suffix = filename.split(".")
+            suffix = "." + suffix
+            local_file = NamedTemporaryFile(
+                suffix=suffix, prefix=prefix, dir=outdir, delete=False
+            )
+            local_file.write(r.content)
+            filedata[i] = filedata[i].replace(url, local_file.name)


While this is fine, we generally don't want to use indices if we can help it. It is more understandable to iterate through the iterable object : for data in filedata:....

I used the index because I wasn't sure if the assignment in line 63 would modify the filedata list if I iterated through each element.

nikola-rados · 2020-08-31T18:23:59Z

osprey/utils.py

+            local_file = NamedTemporaryFile(
+                suffix=suffix, prefix=prefix, dir=outdir, delete=False
+            )


Great use of tempfile here 👍

nikola-rados · 2020-08-31T18:24:19Z

osprey/utils.py

+    write_config = open(config, "w")
+    for line in filedata:
+        write_config.write(f"{line}")
+    write_config.close()


Again, we can use a context manager.

nikola-rados · 2020-08-31T18:30:03Z

requirements.txt

@@ -2,6 +2,6 @@ pywps>=4.2
 jinja2
 click
 psutil
-rvic==1.1.0post1
+rvic==1.1.1


nikola-rados · 2020-08-31T18:30:42Z

requirements_dev.txt

@@ -1,6 +1,7 @@
 pytest
 flake8
 pytest-flake8
+requests-mock


I didn't know this package existed. I wonder if we can apply to our notebooks to make truly offline tests.

nikola-rados · 2020-08-31T18:31:43Z

tests/configs/parameter_https.cfg

@@ -73,7 +73,7 @@ SEARCH_FOR_CHANNEL: False
 #-- Path to Pour Points File (char) --#
 # A comma separated file of outlets to route to [lons, lats] - one coordinate pair per line (order not important)
 # May optionally include a column [names] - which will (if not aggregating) be included in param file
-FILE_NAME: https://docker-dev03.pcic.uvic.ca/twitcher/ows/proxy/thredds/dodsC/datasets/RVIC/sample_pour.txt
+FILE_NAME: https://docker-dev03.pcic.uvic.ca/twitcher/ows/proxy/thredds/fileServer/datasets/RVIC/sample_pour.txt


What is the purpose of this change from dodsC to fileServer?

This is so that https instead of OpenDAP is used to download the file from THREDDS. James told me that it makes downloading entire files easier since I won't have to specify attributes or time ranges.

nikola-rados · 2020-08-31T18:32:13Z

tests/conftest.py

+import pytest
+from osprey.testing import make_mock_urls
+
+
+@pytest.fixture
+def conftest_make_mock_urls(config, requests_mock):
+    return make_mock_urls(config, requests_mock)


Nice fixture!

nikola-rados · 2020-08-31T18:34:54Z

tests/test_wps_parameters.py

+        read_config = open(config, "r")
+        temp_config.writelines(read_config.read())
+        temp_config.read()


Is there a reason we aren't just directly reading into the temp_config?

I'm not sure what you mean here.

eyvorchuk · 2020-08-31T18:58:29Z

That being said I am getting failures on my pytest runs locally.

What do the FILE_NAMEs in your .cfg files look like?

nikola-rados · 2020-08-31T19:06:00Z

FILE_NAME: https://docker-dev03.pcic.uvic.ca/twitcher/ows/proxy/thredds/fileServer/datasets/RVIC/sample_flow_parameters.nc

eyvorchuk · 2020-08-31T19:08:32Z

And in sample_parameter_config.cfg? Or is it just the https one that's failing?

nikola-rados · 2020-08-31T19:13:34Z

FILE_NAME: tests/data/samples/sample_pour.txt

And here are the failures I got:

==================================================================================================== FAILURES =====================================================================================================
____________________________________________________________ test_wps_convolution[/home/nrados/github/pcic/osprey/tests/configs/convolve_opendap.cfg] _____________________________________________________________
Traceback (most recent call last):
  File "/home/nrados/github/pcic/osprey/tests/test_wps_convolution.py", line 38, in test_wps_convolution
    run_wps_process(Convolution(), params)
  File "/home/nrados/github/pcic/osprey/venv/lib/python3.6/site-packages/wps_tools/testing.py", line 65, in run_wps_process
    assert_response_success(resp)
  File "/home/nrados/github/pcic/osprey/venv/lib/python3.6/site-packages/pywps/tests.py", line 145, in assert_response_success
    assert len(success) == 1
AssertionError
---------------------------------------------------------------------------------------------- Captured stderr call -----------------------------------------------------------------------------------------------
curl error details: 
__________________________________________________________________________________________ test_wps_convolution[config1] __________________________________________________________________________________________
Traceback (most recent call last):
  File "/home/nrados/github/pcic/osprey/tests/test_wps_convolution.py", line 38, in test_wps_convolution
    run_wps_process(Convolution(), params)
  File "/home/nrados/github/pcic/osprey/venv/lib/python3.6/site-packages/wps_tools/testing.py", line 65, in run_wps_process
    assert_response_success(resp)
  File "/home/nrados/github/pcic/osprey/venv/lib/python3.6/site-packages/pywps/tests.py", line 145, in assert_response_success
    assert len(success) == 1
AssertionError
---------------------------------------------------------------------------------------------- Captured stderr call -----------------------------------------------------------------------------------------------
curl error details:

eyvorchuk · 2020-08-31T19:22:54Z

Oh, convolution is @sum1lim's process. I didn't modify anything involved with it in the PR, so the issues might be happening elsewhere.

sum1lim · 2020-08-31T19:50:54Z

@nikola-rados if only convolution process is not working, the possibility is that valid_ranges are not changed in share.py. Could you double-check it?

sum1lim · 2020-08-31T21:11:16Z

OK, I know the problem. @eyvorchuk, could you change this line to following?

"DATL_LIQ_FLDS": ["RUNOFF", "BASEFLOW"],

I actually encountered this problem after RVIC 1.1.1 is used.

eyvorchuk · 2020-08-31T21:18:45Z

Works now.

…ing config files

Eric Yvorchuk added 7 commits August 21, 2020 14:35

Use https urls in config file

1857448

Use https url for uhbox.csv and format other files with black

5436a19

Read config file in test_parameters_https

1807593

Use requests_mock for urls

15b748d

Place make_mock_urls in conftest.py

d2a66bb

Remove requests_mock import in conftest

13fe89d

Remove wget requirement

4283862

eyvorchuk requested review from jameshiebert and sum1lim August 24, 2020 17:03

sum1lim reviewed Aug 24, 2020

View reviewed changes

sum1lim suggested changes Aug 24, 2020

View reviewed changes

Eric Yvorchuk added 4 commits August 24, 2020 13:50

Remove new line character when writing back to config file

dba1546

Move make_mock_urls to osprey/testing and import in conftest

d6772ea

Use config file with mixed file paths for demo

d7ee5c3

Change rvic requirement to 1.1.1

9eb95c2

eyvorchuk requested a review from nikola-rados August 31, 2020 17:08

nikola-rados suggested changes Aug 31, 2020

View reviewed changes

Revert number of parallel processors and use context manager for open…

8f71878

…ing config files

nikola-rados merged commit 87225b5 into master Sep 1, 2020

nikola-rados deleted the i26-use-urls branch September 1, 2020 16:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

I26 use urls #27

I26 use urls #27

eyvorchuk commented Aug 24, 2020

eyvorchuk commented Aug 24, 2020 •

edited

Loading

sum1lim Aug 24, 2020 •

edited

Loading

sum1lim left a comment •

edited

Loading

sum1lim Aug 24, 2020

sum1lim Aug 24, 2020

eyvorchuk Aug 24, 2020

eyvorchuk Aug 24, 2020

eyvorchuk Aug 24, 2020

sum1lim Aug 24, 2020 •

edited

Loading

jameshiebert commented Aug 24, 2020 •

edited

Loading

sum1lim commented Aug 24, 2020

eyvorchuk commented Aug 24, 2020

nikola-rados left a comment

nikola-rados Aug 31, 2020

nikola-rados Aug 31, 2020

nikola-rados Aug 31, 2020

eyvorchuk Aug 31, 2020

nikola-rados Aug 31, 2020

nikola-rados Aug 31, 2020

nikola-rados Aug 31, 2020

nikola-rados Aug 31, 2020

nikola-rados Aug 31, 2020

eyvorchuk Aug 31, 2020

nikola-rados Aug 31, 2020

nikola-rados Aug 31, 2020

eyvorchuk Aug 31, 2020

eyvorchuk commented Aug 31, 2020

nikola-rados commented Aug 31, 2020

eyvorchuk commented Aug 31, 2020 •

edited

Loading

nikola-rados commented Aug 31, 2020

eyvorchuk commented Aug 31, 2020

sum1lim commented Aug 31, 2020

sum1lim commented Aug 31, 2020

eyvorchuk commented Aug 31, 2020

		@@ -73,7 +73,7 @@ SEARCH_FOR_CHANNEL: False
		#-- Path to Pour Points File (char) --#

I26 use urls #27

I26 use urls #27

Conversation

eyvorchuk commented Aug 24, 2020

eyvorchuk commented Aug 24, 2020 • edited Loading

sum1lim Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

sum1lim left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sum1lim Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

jameshiebert commented Aug 24, 2020 • edited Loading

sum1lim commented Aug 24, 2020

eyvorchuk commented Aug 24, 2020

nikola-rados left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyvorchuk commented Aug 31, 2020

nikola-rados commented Aug 31, 2020

eyvorchuk commented Aug 31, 2020 • edited Loading

nikola-rados commented Aug 31, 2020

eyvorchuk commented Aug 31, 2020

sum1lim commented Aug 31, 2020

sum1lim commented Aug 31, 2020

eyvorchuk commented Aug 31, 2020

eyvorchuk commented Aug 24, 2020 •

edited

Loading

sum1lim Aug 24, 2020 •

edited

Loading

sum1lim left a comment •

edited

Loading

sum1lim Aug 24, 2020 •

edited

Loading

jameshiebert commented Aug 24, 2020 •

edited

Loading

eyvorchuk commented Aug 31, 2020 •

edited

Loading