Fix file not found output message #8324

ajscimone · 2020-09-30T03:26:56Z

Pull request overview

Fixes EnergyPlus does not report a clear message about where it attempted to find files which were not found. #8282
This pull request fixes an issue with the DataSystemVariables::CheckForActualFileName function not outputting any information about where EnergyPlus searched for files it was unable to find. After this pull request, users can now pass in an optional vector which will be populated with locations that EnergyPlus looks for the file being checked, and a flag to ask for the function to output the checked locations as error messages. This will help users quickly identify why EnergyPlus cannot locate the files they are referencing in an idf.

The original idf file which caught this bug is posted here:
in.idf.txt

To demonstrate the effect of this pull request here is the .err output from develop on the above idf before this pull request:
eplusout.err.txt

And here is the .err file for this idf after this pull request.
eplusout.err.txt

The result is a much clearer output in the err stream about the issues with the files not being found, as well as a more understandable fatal error.

Additionally, these two idfs also use this file checking function, and so additional .err outputs can be seen by running them (provided you don't let EnergyPlus find the actual files it's looking for.)

ScheduleFileTest.idf.txt
SolarShadingTest.idf.txt

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

Reviewer

This will not be exhaustively relevant to every PR.

…and output as error messages if the file is not found.

… of locations to check for a file instead of checking them with multiple blocks of code.

… the file not found error outputs.

…flag is passed.

…resent.

src/EnergyPlus/DataSystemVariables.cc

…n optional array to be filled with locations checked for, as well as the boolean value for outputting to the err file when a file is not found. This behavior of printing a list of locations checked when a file is not found to the err file is now always present. Additionally, a new string can optionally be passed to the function as a context string which will modify this printing output to give context about where this file missing output is coming from. Lastly, the table files object in curve manager has been modified to use this structure.

…ings to each, allowing for cleanup of error messages in these sections of code.

…kable for a cross platform situation and using the error stream matching function to detect the issue tested for is actually being printed in the err stream.

nealkruis · 2020-10-01T17:36:15Z

src/EnergyPlus/DataSystemVariables.cc

+            {InputFileName, "InputFileName"},
+            {DataStringGlobals::inputDirPathName + InputFileName, "inputDirPathName"},
+            {envinputpath1 + InputFileName, "envinputpath1"},
+            {envinputpath2 + InputFileName, "envinputpath2"},
+            {envprogrampath + InputFileName, "envprogrampath"},
+            {CurrentWorkingFolder + InputFileName, "CurrentWorkingFolder"},
+            {ProgramPath + InputFileName, "ProgramPath"}


@ajscimone Here are more informative descriptions for each of the strings that describe where files are searched:

Current working directory

IDF directory

"epin" environment variable

"input_path" environment variable

<This variable is never used or set, I think it's supposed to be "program_path" environment variable initialized as cProgramPath, but that variable overwrites ProgramPath instead>

INI directory (if it exists) <from what I can tell this is always identical to [1] -- maybe we can drop it?>

"program" > "dir" from INI file

We should maybe add exeDirectory to the paths searched, too? I think that is the intent of [5] and [7], but neither of those seem reliable.

@Myoldmopar @mjwitte thoughts on any of this?

Separate question: Does/should this interact with the way python plug-in paths are searched?

… not found errors, and fix the logic flow for file not found searches in CurveManager to not throw a fatal error immediately when a file is not found. This results in more output from the Curve Manager about additional files it could not find, and a more reasonable fatal error.

Myoldmopar

I'm OK with these changes, it looks like it won't affect the Python stuff at all, and it looks to be in good shape. I had a couple suggestions in case additional changes are needed, but otherwise it's fine as-is. I haven't tested it, I think @mitchute is going to verify the fix, but the code itself looks good.

Myoldmopar · 2020-11-11T16:55:37Z

src/EnergyPlus/CurveManager.cc

@@ -1750,9 +1750,13 @@ namespace CurveManager {
                            std::size_t rowNum = indVarInstance.at("external_file_starting_row_number").get<std::size_t>() - 1;

                            if (!state.dataCurveManager->btwxtManager.tableFiles.count(filePath)) {
-                                state.dataCurveManager->btwxtManager.tableFiles.emplace(filePath, TableFile{state, filePath});
+                                TableFile tableFile;
+                                ErrorsFound |= tableFile.load(state, filePath);


Breaking the initialization into a separate bool load() function makes sense.

Myoldmopar · 2020-11-11T16:57:51Z

src/EnergyPlus/CurveManager.cc

+        std::string contextString = "CurveManager::TableFile::load: ";
+        DataSystemVariables::CheckForActualFileName(state, path, fileFound, fullPath, contextString);
+        if(!fileFound){
+            return true;


So true means error. OK. A little confusing maybe but not a big deal. If other commits are needed here for anything, I'd suggest considering a clearer convention like bool loadSuccess() where true is clearly success. Anyway, not a big deal if it's working.

I was kind of thinking of it as an error code return where 0/false means success.

Myoldmopar · 2020-11-11T17:01:06Z

src/EnergyPlus/DataSystemVariables.cc

+            {envprogrampath + InputFileName, "\"program_path\" Environment Variable"},
+            {CurrentWorkingFolder + InputFileName, "INI File Directory"},
+            {ProgramPath + InputFileName, "\"program\", \"dir\" from INI File"}
+        };


This is quite a small list, so it's not a big deal, but an array would've been acceptable here since I think this vector is staying fixed length.

Myoldmopar · 2020-11-11T17:01:43Z

src/EnergyPlus/ExternalInterface.cc

+
+                std::string contextString = cCurrentModuleObject + ", " + cAlphaFieldNames(1) + ": ";
+
+                CheckForActualFileName(state, cAlphaArgs(1), fileExist, tempFullFileName, contextString);


This makes sense, just needing to add a new context string.

Myoldmopar · 2020-11-11T17:01:56Z

src/EnergyPlus/ExternalInterface.cc

@@ -1165,8 +1169,6 @@ namespace ExternalInterface {
                    }
                    fullFileName(Loop) = tempFullFileName;
                } else {
-                    ShowSevereError(state, "ExternalInterface/InitExternalInterfaceFMUImport:");
-                    ShowContinueError(state, "file not located = \"" + cAlphaArgs(1) + "\".");


And then no longer needing to report the error details.

mitchute · 2020-11-11T18:06:41Z

Confirmed that this works. One minor cosmetic issue that we might consider cleaning up if there are other commits required:

Break the "Paths searched:" string, here (https://github.com/NREL/EnergyPlus/pull/8324/files#diff-c3b9eb9daf6e23f579ad991f364fc70e9416d16a05619e9cc830960094e3bbb6R284) out into a separate ShowContinueError. As it is, "Paths Searched:" comes out in the last error summary.

**  Fatal  ** GetCurveInput: Errors found in getting Curve Objects.  Preceding condition(s) cause termination.
   ...Summary of Errors that led to program termination:
   ..... Reference severe error count=4
   ..... Last severe error=CurveManager::TableFile::load: "../performanceTable_PI_168H.csv" not found. Paths searched:

mitchute · 2020-11-12T17:32:17Z

I just confirmed the changes locally. The Kiva unit test failures aren't related to this and I think we're investigating that separately anyway, so I'm going to merge this.

mjwitte · 2020-11-12T22:11:33Z

I'm guessing this PR is the source of audit diffs that show up now on test files that access an external file. e.g. HybridModel_4Zone_Solve_PeopleCount_with_HVAC and 1ZoneUncontrolled_win_1

mitchute · 2020-11-12T22:18:28Z

Seems right to me. Comparing the diffs, the only real difference in the messages are the clone branch paths.

i.e. clone_baseline vs. clone_branch

mjwitte · 2020-11-12T22:25:19Z

So, we'll need to add some rules to CI to understand that these are not substantive diffs and not warn. Otherwise, we'll never get a fully green report.

Myoldmopar · 2020-11-13T15:21:16Z

Got it: NREL/EnergyPlusRegressionTool@d04aae5

Once that PR merged, Decent CI will not show those diffs anymore.

ajscimone added 9 commits September 16, 2020 16:28

Create a vector to track locations tested for a file while searching …

c4f656d

…and output as error messages if the file is not found.

Merge branch 'develop' into fix-file-not-found-output-message

80e4d8f

Merge branch 'develop' into fix-file-not-found-output-message

2ab0ac8

Refactor the CheckForActualFileName function to loop through an array…

22126fc

… of locations to check for a file instead of checking them with multiple blocks of code.

Add a unit test for the CheckForActualFileName function which catches…

578d433

… the file not found error outputs.

Merge branch 'develop' into fix-file-not-found-output-message

8f6e5ea

Add a flag to file checking function to output error messages if the …

8a3db61

…flag is passed.

Fix output error where printing of file searching errors was always p…

75cd34d

…resent.

Fix variable access issues in check for actual file.

e91271b

ajscimone requested a review from nealkruis September 30, 2020 03:27

ajscimone added Defect Includes code to repair a defect in EnergyPlus NotIDDChange Code does not impact IDD (can be merged after IO freeze) labels Sep 30, 2020

ajscimone self-assigned this Sep 30, 2020

Myoldmopar reviewed Sep 30, 2020

View reviewed changes

src/EnergyPlus/DataSystemVariables.cc Outdated Show resolved Hide resolved

ajscimone added 6 commits September 30, 2020 16:44

Clean up the other uses of CheckForActualFileName and add context str…

31a59c4

…ings to each, allowing for cleanup of error messages in these sections of code.

Remove a few unneeded comments from the code base.

631eb86

Clean up some issues with the file not found unit test, making it wor…

11911ee

…kable for a cross platform situation and using the error stream matching function to detect the issue tested for is actually being printed in the err stream.

Merge branch 'develop' into fix-file-not-found-output-message

62d43df

Add the variable being searched through to the output string.

f04d3a3

nealkruis reviewed Oct 1, 2020

View reviewed changes

ajscimone added 2 commits October 1, 2020 15:32

Fix the expected error in the file not found unit test.

b0ed48c

ajscimone marked this pull request as ready for review October 1, 2020 21:42

nealkruis mentioned this pull request Oct 2, 2020

getAbsolutePath strips characters from the file name #8329

Closed

2 tasks

ajscimone and others added 2 commits October 14, 2020 14:33

Merge develop.

296a9b9

Merge develop.

5438bb1

Myoldmopar approved these changes Nov 11, 2020

View reviewed changes

Remove unused environment variable.

4a93d7e

nealkruis added 2 commits November 11, 2020 13:35

Convert to std::array.

f1bcbbd

Fix error message line break.

fbdcded

mitchute merged commit 283b640 into NREL:develop Nov 12, 2020

matthew-larson deleted the fix-file-not-found-output-message branch May 12, 2021 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix file not found output message #8324

Fix file not found output message #8324

ajscimone commented Sep 30, 2020 •

edited

Loading

nealkruis Oct 1, 2020

Myoldmopar left a comment

Myoldmopar Nov 11, 2020

Myoldmopar Nov 11, 2020

nealkruis Nov 11, 2020

Myoldmopar Nov 11, 2020

Myoldmopar Nov 11, 2020

Myoldmopar Nov 11, 2020

mitchute commented Nov 11, 2020

mitchute commented Nov 12, 2020

mjwitte commented Nov 12, 2020

mitchute commented Nov 12, 2020

mjwitte commented Nov 12, 2020

Myoldmopar commented Nov 13, 2020


		std::string contextString = cCurrentModuleObject + ", " + cAlphaFieldNames(1) + ": ";

		CheckForActualFileName(state, cAlphaArgs(1), fileExist, tempFullFileName, contextString);

Fix file not found output message #8324

Fix file not found output message #8324

Conversation

ajscimone commented Sep 30, 2020 • edited Loading

Pull request overview

Pull Request Author

Reviewer

nealkruis Oct 1, 2020

Choose a reason for hiding this comment

Myoldmopar left a comment

Choose a reason for hiding this comment

Myoldmopar Nov 11, 2020

Choose a reason for hiding this comment

Myoldmopar Nov 11, 2020

Choose a reason for hiding this comment

nealkruis Nov 11, 2020

Choose a reason for hiding this comment

Myoldmopar Nov 11, 2020

Choose a reason for hiding this comment

Myoldmopar Nov 11, 2020

Choose a reason for hiding this comment

Myoldmopar Nov 11, 2020

Choose a reason for hiding this comment

mitchute commented Nov 11, 2020

mitchute commented Nov 12, 2020

mjwitte commented Nov 12, 2020

mitchute commented Nov 12, 2020

mjwitte commented Nov 12, 2020

Myoldmopar commented Nov 13, 2020

ajscimone commented Sep 30, 2020 •

edited

Loading