Supporting TECA_assets svn repo in TECA by elbashandy · Pull Request #351 · LBL-EESA/TECA

elbashandy · 2020-04-29T21:55:50Z

Moved deeplab models + png/jpg files to TECA_assets

Resolves #477

burlen · 2020-05-01T17:07:44Z

it's looking good so far. We need to add a cmake target to fetch assets during build. So for instance one could say "make TECA_assets" and this would trigger a svn co or svn up. Would also like such a target for TECA_data. Both these targets need to be configurable to be in or out of the default target.

See also:
https://cmake.org/cmake/help/latest/module/ExternalProject.html
https://cmake.org/cmake/help/v3.17/module/FetchContent.html

elbashandy · 2020-05-08T03:01:40Z

@burlen I set TECA_DATA_REQ to be our flag that we can set if we want to TECA_data to be downloaded. Do you think something like this could work?

set(TECA_DATA_REQ OFF CACHE BOOL "require TECA_data or not")
if (TECA_DATA_REQ)
    FetchContent_Declare(
        TECA_data
        SVN_REPOSITORY svn://svn.code.sf.net/p/teca/TECA_data
    )
    FetchContent_GetProperties(TECA_data)
    if(NOT TECA_data_POPULATED)
        FetchContent_Populate(TECA_data)
        add_subdirectory(${TECA_data_SOURCE_DIR})
    endif()

    set(TECA_DATA_ROOT ${TECA_data_SOURCE_DIR})
else()
    set(TECA_DATA_ROOT "/path/to/TECA_data"
        CACHE PATH "Path to TECA_data repository")
endif()

set(tmp OFF)
if (EXISTS "${TECA_DATA_ROOT}")
    set(tmp ON)
    message(STATUS "TECA_data -- available")
elseif (REQUIRE_TECA_DATA)
    message(FATAL_ERROR "TECA_data -- required but not found")
else()
    message(STATUS "TECA_data -- not available")
endif()
set(TECA_HAS_DATA ${tmp} CACHE BOOL "TECA_data is present")

burlen

Looks good (but please notice that I flagged minor tweak in the code comments). Also I don't see the cmake code to fetch the repo. This would be needed for deployment on PyPi. Comments above have links to the relevant cmake functions. I'd like to see this incorporated.

burlen

Hi Abdel, solid progress was made, but I flagged a few issues. In addition to comments in the code, also need to install the assets by make install.

burlen · 2020-09-21T23:13:50Z

+            CONFIGURE_COMMAND "" BUILD_COMMAND ""
+            INSTALL_COMMAND "" LOG_DOWNLOAD 1)
+        ExternalProject_Get_property(TECA_assets SOURCE_DIR)
+        set(TECA_ASSETS_ROOT ${SOURCE_DIR})


I believe that this will make a new variable and will not update the cached variable of the same name declared above. The cmake docs confirm this. This code should be restructured so the second variable is not needed. https://cmake.org/cmake/help/v3.18/manual/cmake-language.7.html#cmake-language-variables

burlen · 2020-09-21T23:30:23Z

-    echo "usage: test_deeplabv3p_ar_detect_app.sh [app prefix] " \
-         "[data root] [mpi exec] [test cores]"
+    echo "usage: test_deeplabv3p_ar_detect_app.sh [app prefix] "  \
+         "[data root] [assets root] [mpi exec] [test cores]"


we should no longer have to pass this on the command line , use the functions you added in teca_py_config.i

I applied this change only to the test_deeplabv3p_ar_detect.py test in 8a8bdb. However I didn't do this to the app because the user is expected to pass the deeplab model path to the command line --pytorch_deeplab_model. That's why [assets root] is being passed to this bash file

You need to update the command line application. You could leave the command line option, so that one could pass the model there. The default should be that the model is taken from the assets directory. Having the model available internally(in the install) is the point of deploying the assets with TECA and the app should make use of it.

burlen · 2020-09-21T23:32:54Z


    mpiexec -np 10 ./bin/teca_tc_trajectory_scalars   \
-        --texture ../../TECA_data/earthmap4k.png      \
+        --texture ../../TECA_assets/earthmap4k.png      \


we no longer need to pass on the command line, the default value can be obtained using the new api you defined in teca_py_config.i

I don't think we should do that because this is given as a command line argument to the --texture option in the teca_tc_trajectory_scalars app. We can modify it to /path/to/background_image.png if that makes sense. Please let me know if I am missing something.

You might be missing that the point of deploying the assets with TECA is that we no longer need to pass these on the command line. It's fine to leave the command line options but make them optional, with the default value pointing to the assets location. Same deal as with the pytorch model.

burlen · 2020-09-21T23:43:16Z

+            SVN_REPOSITORY svn://svn.code.sf.net/p/teca/TECA_data
+            UPDATE_COMMAND svn update
+            CONFIGURE_COMMAND "" BUILD_COMMAND ""
+            INSTALL_COMMAND "" LOG_DOWNLOAD 1)


how verbose is this? is it disruptive to display in the terminal? if it is not too bad doing so may help a user understand what's going on. I'll leave that call up to you.

Yes it is a little disruptive especially to the progress percentage-values that we get at the building process.

burlen · 2020-09-21T23:45:11Z

 \begin{minted}{bash}
 mpiexec -np 10 ./bin/teca_tc_trajectory_scalars   \
-    --texture ../../TECA_data/earthmap4k.png      \
+    --texture ../../TECA_assets/earthmap4k.png      \


use the new api in teca_py_config.i to fetch a default internally

I don't think we should do that because this is given as a command line argument to the --texture option in the teca_tc_trajectory_scalars app. We can modify it to /path/to/background_image.png if that makes sense. Please let me know if I am missing something.

see comments above.

burlen · 2020-09-21T23:55:17Z

+            CONFIGURE_COMMAND "" BUILD_COMMAND ""
+            INSTALL_COMMAND "" LOG_DOWNLOAD 1)
+        ExternalProject_Get_property(TECA_data SOURCE_DIR)
+        set(TECA_DATA_ROOT ${SOURCE_DIR})


restructure this as explained in section re: assets below

…ing it directly

elbashandy requested a review from burlen April 29, 2020 22:20

elbashandy force-pushed the teca_assets_svn_support branch from d6b1d4f to e0657f5 Compare May 8, 2020 01:34

elbashandy force-pushed the teca_assets_svn_support branch from e0657f5 to ae8e18f Compare May 12, 2020 00:13

taobrienlbl added the 1_high_priority an issue that should be fixed prior to the next release label Jun 19, 2020

burlen requested changes Jun 29, 2020

View reviewed changes

Comment thread test/travis_ci/install_osx.sh Outdated

elbashandy changed the title ~~Supporting TECA_assets svn repo in TECA~~ [WIP] Supporting TECA_assets svn repo in TECA Sep 16, 2020

elbashandy force-pushed the teca_assets_svn_support branch 2 times, most recently from 0b2a677 to ca41579 Compare September 18, 2020 21:20

elbashandy changed the title ~~[WIP] Supporting TECA_assets svn repo in TECA~~ Supporting TECA_assets svn repo in TECA Sep 18, 2020

elbashandy force-pushed the teca_assets_svn_support branch from ca41579 to 27ac12d Compare September 18, 2020 21:39

elbashandy requested a review from burlen September 18, 2020 23:01

burlen removed the 1_high_priority an issue that should be fixed prior to the next release label Sep 21, 2020

burlen requested changes Sep 21, 2020

View reviewed changes

elbashandy force-pushed the teca_assets_svn_support branch 3 times, most recently from 8a8bdb2 to 29a5bd9 Compare October 10, 2020 00:07

elbashandy added 3 commits October 9, 2020 18:14

Supporting TECA_data to be a separate target

97e57e9

Supporting TECA_assets svn repo in TECA

4dd9b18

Loading deeplab model using get_teca_assets_root() instead of specify…

0d1cb67

…ing it directly

elbashandy force-pushed the teca_assets_svn_support branch from 29a5bd9 to 0d1cb67 Compare October 10, 2020 01:15

elbashandy requested a review from burlen October 11, 2020 18:24

Uh oh!

Conversation

elbashandy commented Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

burlen commented May 1, 2020

Uh oh!

elbashandy commented May 8, 2020

Uh oh!

burlen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

burlen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

burlen Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

elbashandy commented Apr 29, 2020 •

edited

Loading

burlen Nov 18, 2020 •

edited

Loading