Move the application data into the git repo

[scsides]

Description
The ISIS application specific data (e.g., PDS3 to ISIS cube translation) are currently mixed in with the general data (e.g., Cassini/kernels/ck/*). Move the application specific data into the git repository and out of the ISIS data area.

Addition:
The changes proposed to have many, not all, text files under revision control include:

• During build and install steps: Move files to: $ISISROOT/appdata/... from the src tree
  • Move the template files
  • Move translation files
  • Move serial number files

Example

[KrisBecker]

Originally in #3728.

While I agree this is technically not API breaking as defined by the current policy, it potentially breaks a lot of stuff. It appears this type of change should be covered by policy. Perhaps a period of deprecation could be specified where both environment variables, ISIS3DATA and ISISDATA, refer to the same value to ease the transition and provide an environment in which users can assess the impact in their environments. It might be a good idea to treat this version as a "breaking change" release.

Here are but a few potential issues I have identified:

ISIS3DATA is hardcode in the ISIS appdata branch

Most of these occurrences are in unit tests, some are in main applications and others are in comments and should probably be changed for consistency. However, in some cases, the change would not be backward compatible (is this a breaking change?). Its likely these issues have already been identified, but here is a list of the occurrences in the appdata branch that directly use $ISIS3DATA:

(isis3/appdata)$ find ./isis/src -name '*.cpp' -exec grep -A1 -C0 -H ISIS3DATA {} \;
./isis/src/voyager/apps/voy2isis/main.cpp:  QString lsk = "$ISIS3DATA/base/kernels/lsk/naif????.tls";
./isis/src/voyager/apps/voy2isis/main.cpp:  QString sclk = "$ISIS3DATA/voyager";
./isis/src/tgo/objs/TgoCassisCamera/TgoCassisDistortionMap.cpp:    // of the CCD. (See $ISIS3DATA/tgo/assets/distortion/DistortionModelA3CorrRoots.jpg).
./isis/src/tgo/objs/TgoCassisCamera/TgoCassisDistortionMap.cpp:    // of the CCD. (See $ISIS3DATA/tgo/assets/distortion/DistortionModelA3DistRoots.jpg).
./isis/src/rosetta/objs/RosettaVirtisCamera/unitTest.cpp:    Cube visCube("$ISIS3DATA/rosetta/testData/V1_00388238556.cub", "r");
./isis/src/rosetta/objs/RosettaVirtisCamera/unitTest.cpp:    Cube irCube("$ISIS3DATA/rosetta/testData/I1_00382172310.cub", "r");
./isis/src/rosetta/apps/rosvirtis2isis/main.cpp:    QString sclk = "$ISIS3DATA/rosetta/kernels/sclk/ROS_??????_STEP.TSC"; 
./isis/src/rosetta/apps/rosvirtis2isis/main.cpp:    QString lsk  = "$ISIS3DATA/base/kernels/lsk/naif????.tls"; 
./isis/src/viking/apps/viknobutter/main.cpp:  QString maskParameter = "$ISIS3DATA/viking" + toString(spn) +
./isis/src/system/apps/dempack/main.cpp:      QString izpackFile = fileList[j].replace("ISIS3DATA", "{ENV[ISIS3DATA]}");
./isis/src/messenger/objs/MdisCamera/MdisCamera.cpp:    // in $ISIS3DATA/messenger/kernels/iak/mdisAddendum???.ti for the
./isis/src/messenger/objs/MdisCamera/MdisCamera.cpp:    // See $ISIS3DATA/messenger/kernels/iak/mdisAddendumXXX.ti or possibly
./isis/src/messenger/objs/MdisCamera/MdisCamera.cpp:    // $ISIS3DATA/messenger/kernels/ik/msgr_mdis_vXXX.ti for the *_OD_K
./isis/src/socet/apps/socetlinescankeywords/main.cpp:  //      of 0.007 mm gotten from $ISIS3DATA/mgs/kernels/ik/moc20.ti.  (The
./isis/src/newhorizons/apps/mvic2isis/main.cpp:  QString lsk = "$ISIS3DATA/base/kernels/lsk/naif????.tls";
./isis/src/newhorizons/apps/mvic2isis/main.cpp:  QString sclk = "$ISIS3DATA/newhorizons/kernels/sclk/new_horizons_???.tsc";
./isis/src/control/apps/cnetstats/cnetstats.cpp:        Pvl pvlTemplate("$ISIS3DATA/base/templates/cnetstats/cnetstats.def");
./isis/src/control/apps/cnetref/main.cpp:        pvlTemplate = Pvl("$ISIS3DATA/base/templates/cnetref/cnetref_operator.def");
./isis/src/control/apps/cnetref/main.cpp:        pvlTemplate = Pvl("$ISIS3DATA/base/templates/cnetref/cnetref_nooperator.def");
./isis/src/control/apps/cnetedit/main.cpp:      Pvl pvlTemplate("$ISIS3DATA/base/templates/cnet_validmeasure/validmeasure.def");
./isis/src/base/objs/Pipeline/unitTest.cpp:  ui.PutFileName("FROM",  "$ISIS3DATA/odyssey/testData/I00831002RDR.even.cub");
./isis/src/base/objs/Pipeline/unitTest.cpp:  ui.PutFileName("FROM2", "$ISIS3DATA/odyssey/testData/I00831002RDR.odd.cub");
./isis/src/base/objs/Pipeline/unitTest.cpp:  ui.PutAsString("FROM", "$ISIS3DATA/odyssey/testData/I00831002RDR.cub");
./isis/src/base/objs/Pipeline/unitTest.cpp:  pc1.SetInputFile(FileName("$ISIS3DATA/mro/testData/PSP_001446_1790_BG12_0.cub"));
./isis/src/base/objs/Pipeline/unitTest.cpp:  pc2.SetInputFile(FileName("$ISIS3DATA/mro/testData/PSP_001446_1790_BG12_0.cub"));
./isis/src/base/objs/EmbreeTargetManager/unitTest.cpp:    QString copyDSKFile = "$ISIS3DATA/base/testData/hay_a_amica_5_itokawashape_v1_0_64q.bds";
./isis/src/base/objs/ShapeModelFactory/unitTest.cpp:    QString inputFile = "$ISIS3DATA/mgs/testData/ab102401.cub";
./isis/src/base/objs/ShapeModelFactory/unitTest.cpp:    inputFile = "$ISIS3DATA/galileo/testData/1213r.cub";
./isis/src/base/objs/ShapeModelFactory/unitTest.cpp:    // inputFile = "$ISIS3DATA/;
./isis/src/base/objs/Sensor/unitTest.cpp:                        "$ISIS3DATA/base/dems/molaMarsPlanetaryRadius0004.cub");
./isis/src/base/objs/Sensor/unitTest.cpp:            "$ISIS3DATA/base/dems/molaMarsPlanetaryRadius0004.cub" << endl;
./isis/src/base/objs/Sensor/unitTest.cpp:            "ShapeModel=$ISIS3DATA/base/dems/molaMarsPlanetaryRadius0004.cub"
./isis/src/base/objs/Sensor/unitTest.cpp:            "ShapeModel=$ISIS3DATA/base/dems/molaMarsPlanetaryRadius0004.cub"
./isis/src/base/objs/Sensor/unitTest.cpp:            "ShapeModel=$ISIS3DATA/base/dems/molaMarsPlanetaryRadius0004.cub"
./isis/src/base/objs/Sensor/unitTest.cpp:            "ShapeModel=$ISIS3DATA/base/dems/molaMarsPlanetaryRadius0004.cub"
./isis/src/base/objs/Gruen/Gruen.cpp:   * @brief Load default Gruen parameter file in $ISIS3DATA/base/templates
./isis/src/base/objs/Gui/GuiFilenameParameter.cpp:    QString file = FileName("$ISIS3DATA/base/icons/view_tree.png").expanded();
./isis/src/base/objs/Application/Application.cpp:    QString env6 = "printenv ISIS3DATA >> " + tempFile;

Example of ISIS3DATA in Code

Here is an example of ISIS3DATA in the API. One could still argue this is not API breaking, but its clear the first example is not backward compatible. The second one can be made so by replacing the occurrences of $ISIS3DATA/ with a $.

By the way, I am not a big fan of renaming all the main applications to main.cpp because in some GUI IDEs, its difficult to tell which main application you are editing. I am a SlickEdit user. Which main.cpp tab goes with which source file? Visual aids require a significant amount of screen real estate. Likely this makes things easier to turn ISIS mains into callable functions, or makes things easier in cmake, but it makes some development tasks that were easy and straightforward much more difficult/inconvenient/frustrating/annoying.

Kernel File Management Scripts

Eventually, scripts that manage NAIF kernels in the ISIS environment refer to the install directory. There are many mission kernel download/install scripts that use the $ISIS3DATA environment variable to navigate to the appropriate data directory and generate the kernel DB file, typically with a ./makedb command. These type of scripting scenarios will present challenges as they will require modifications. They will not be backward compatible without additional consideration of explicit support for both variables. I know USGS maintains many of these types of scripts that will require modifications. Our kernel maintenance scripts will require these modifications as well.

Pipeline Processing Scripts

There is likely significant development of ISIS processing scripts that makes use of $ISIS3DATA that will have to change. While there are ways (i.e., IsisPreferences) to specify ISIS data directories, its likely many are not using this overloaded form (e.g., $rosetta/kernels/dsk/ROS_CG_M004_OSPGDLR_U_V1.bds) of file specification in scripts. Unfortunately, this form of ISIS file referencing does not translate into the Unix Shell without use of the $ISIS3DATA environment variable. Changes will still be required that are not readily backward compatible. This will initially make regression testing of previous ISIS versions challenging.

Legacy Use

Note that the ISIS rsync servers has an isis3data module that serve up the ancillary data for ISIS3. This will soon be obsolete, but what happens to ISIS2 on the servers? Note that ISIS2 is no longer maintained, but USGS/Astro is still providing the system through the the rsync server.

And I'm gonna say it - ISIS2 uses the $ISISDATA environment variable. That was the main reason for creating the ISIS3DATA environment variable in ISIS3 - to avoid conflict. This issue will likely serve as the opportunity to wipe ISIS2 off the internet. Its probably time for it, but it needs to be addressed.

IsisPreferences

Perhaps not many users do this, but I always have a copy of IsisPreferences in $HOME/.Isis. (MacTip: I always set GuiHelpBrowser = open so that your user preferred browser is used.) And while this is not recommended practice, I retain much of the contents from the original $ISISROOT/IsisPreferences file including the mission directory specifications that use the $ISIS3DATA environment variable. This will lead to breakage and would likely be difficult for users to determine why they are getting missing file errors.

Summary

This is going to be a potentially painful change for many users, particularly for users who have a long history and substantial investment in ISIS. While the current API definition of "breaking change" may not apply here, it is another form of breakage that should be covered by policy. To ease the transition, it would be nice to provide a period of depreciation of the ISIS3DATA environment variable.

I am not saying this should not be done. Changes like this are necessary/inevitable/painful, but I think we can and should do more to ease the transition.

Thank you for your consideration...

[lwellerastro]

Am I correct in thinking that when this change is implemented, a user would potentially have to re-spice all of their data to update path references in the Kernels Group?

I am not explicitly seeing $ISIS3DATA in path names (I believe that went away some time ago), but in looking at some labels for data across just two of the many projects I am currently working on, I do see /usgs/cpkgs/isis3/data/ in the Kernels Group.

Although many data sets would be straight-forward in re-spicing, I have two projects where images underwent special processing including running deltack. It would be quite painful to deal with those as well as costly time sinks.

[jessemapel]

Perhaps a period of deprecation could be specified where both environment variables, ISIS3DATA and ISISDATA, refer to the same value to ease the transition and provide an environment in which users can assess the impact in their environments.

I think this is a good path forward here. We should have some transition period. Something like a getDataDirectoryPath function that could return ISISDATA if it's set and ISIS3DATA otherwise. Then, after a set amount of time remove the ISIS3DATA part.

Am I correct in thinking that when this change is implemented, a user would potentially have to re-spice all of their data to update path references in the Kernels Group?

The only situation where you need to re-spiceinit your data is if you have $ISIS3DATA in your kernel paths and the data from those kernels is not attached. This is likely an exceedingly small amount of data.

I am not explicitly seeing $ISIS3DATA in path names (I believe that went away some time ago), but in looking at some labels for data across just two of the many projects I am currently working on, I do see /usgs/cpkgs/isis3/data/ in the Kernels Group.

This data would not need to be re-spiceinit'd

[rbeyer]

Perhaps a period of deprecation could be specified where both environment variables, ISIS3DATA and ISISDATA, refer to the same value to ease the transition and provide an environment in which users can assess the impact in their environments.

I think this is a good path forward here. We should have some transition period. Something like a getDataDirectoryPath function that could return ISISDATA if it's set and ISIS3DATA otherwise. Then, after a set amount of time remove the ISIS3DATA part.

This seems like a robust solution: bottleneck the need to know the actual name of the environment variable through a function, and then the function can check for the existence of both.

Of course, this leads to the question of why not always have it this way? Why should the explicit name of an environment variable be sprinkled throughout the codebase? Seems like it should always just be in one place.

[jessemapel]

@rbeyer It is already partially behind an interface. There is a function in the Process classes that gets the mission specific data directories without the caller passing $ISIS3DATA. There are a handful of places that don't use this though.

[scsides]

To expand on what @jessemapel said about re-spiceiniting. If the kernels group of a spiceinit-ed cube has any $ISIS3DATA references, then after the change, re-spiceinit-ing would change those to "$ISISDATA". No real change, except for the environment variable. Both areas can be available at the same time.

Note: The vast majority of the ISIS3DATA and ISISDATA files are identical (i.e., all the SPICE, calibration, DEMs). The files being moved to the source area are mostly translation, PVL formatting, icons and other text files that need to be under revision control alongside the source code that uses them. This way they can be changed if necessary without affecting other versions of ISIS. This was difficult in previous versions.

One case where this could be an issue is if the attached SPICE tables were modified through a jigsaw run and ckwriter or spkwriter were then used and the ISIS3DATA environment variable was not set. These programs re-open the SPICE files to get some additional information before converting the table information back into SPICE files.

[jessemapel]

One case where this could be an issue is if the attached SPICE tables were modified through a jigsaw run and ckwriter or spkwriter were then used and the ISIS3DATA environment variable was not set. These programs re-open the SPICE files to get some additional information before converting the table information back into SPICE files.

It should also be emphasized that you can work around this by setting the ISIS3DATA environment variable to the value of your ISISDATA environment variable before running the applications

[jessemapel]

This is in response to some discussion in a separate PR: #3742 (comment)

@KrisBecker

How is it possible to isolate translation files in the source tree and not ship them prematurely?

This functionality is being dropped because the translation files are tightly connected to the logic of the software. Publicly distributing one and privately distributing the other is of no use to the public. If someone does not want to publicly distributing the translation files, then they should not publicly distribute the software by submitting it to the public repo.

@scsides Will the translation files for O-Rex and EIS be added to the repo?

[KrisBecker]

It is a service to mission teams where, in the past, the USGS provided software support. The team may prefer to hold back translation files and kernels so that they have appropriate time to publish their results before these files are released to the scientific community.

[scsides]

@jessemapel, Tightly coupled is putting it mildly. Many programs/classes will not work with even minor changes to the translations used in the ingest programs. They are code, just not C++. To answer your question, yes they should be moved to the code repo. EDIT: We should ask.

[scsides]

Just to be clear: The kernels are fully controlled by the mission/instrument team. They usually have a release schedule dictated by something like a PDS archive schedule. We are not proposing to move SPICE kernels from the data area, only files directly associated with the code that needs revision control.

[AndrewAnnex]

just to continue some of the conversation about gitlfs in the RFC, has "git-annex" https://git-annex.branchable.com/ been discussed?

here is a good blog post about it vs git lfs: https://lwn.net/Articles/774125/

I have no connection to the project...

Another important consideration before hosting the data in S3 are the bandwidth costs, users should not have to pay for transfer... Also check out the tool rclone https://rclone.org/ for replacing the rsync workflow to whatever is decided upon.

[jessemapel]

Another important consideration before hosting the data in S3 are the bandwidth costs, users should not have to pay for transfer... Also check out the tool rclone https://rclone.org/ for replacing the rsync workflow to whatever is decided upon.

I definitely agree users shouldn't be paying for the transfer costs. I also believe that rclone is being looked into for working with S3.

[AndrewAnnex]

regarding the performance issues with git lfs in the RFC, was that from a test of a prospective user attempting to download the test data using git lfs or just local performance? It could make sense for the usgs folks to user git lfs behind the scenes to manage the data area still and to cut releases of the data that are then synced to buckets or whatever, so even if its slow it could still be useful

[jlaura]

rclone is being looked at.

While I agree that users should not have to pay for data transfer costs, this is not something that we could guarantee one way or another. See the recent NASA OIG report on earth data cloud storage. That said, the current solution is not something that we can continue to pay.

The LFS tests were internal and external. Since we are working to significantly reduce test data volumes and because the data area changes infrequently, we should be good to go using S3 without the need for another layer in between.

[jessemapel]

the data area changes infrequently

I'm not sure what you mean by this. We pull kernel updates from active missions all the time

[jlaura]

I am meaning backwards compatibility related changes, e.g., the bsp change that happened in the last 6 months.

[AndrewAnnex]

just how of curiosity, how much more work will it be to expand the web based spice data and is there a issue tracking those issues? And do the new tech policies have an impact on that service?

[jlaura]

We have a potential project in 2021 to work on migrating additional portions of the code to make use of spice data over the web. This is proposed work. PRs are always solicited!

[jessemapel]

how much more work will it be to expand the web based spice data

We're working towards this goal. Currently the spice web service is just a script on a server that runs spiceinit for you. Next year we are looking to cut down on the number of applications that load kernels and have them read the spice data attached to cubes when available. Then, in the future we can explore changing the spice web service to do more than just run spiceinit.

Improving the spice web service doesn't solve all of these problems, though. There are many non-spice kernel files that are needed such as translation files, calibration files, GUI icons, templates, test files, etc. that we wouldn't want to serve over the web.