Repository: NREL/OpenStudio (language: python)

Issue:
Allow `ScheduleFile` constructor with path instead of ExternalFile argument
Issue overview
--------------
Running the following command on Windows:
`external_file = OpenStudio::Model::ExternalFile::getExternalFile(@model, schedules_path)`

It's throwing an error:
`OS Message: Failed to copy file from "C:/Users/shorowit/AppData/Local/Temp/1/schedules20221212-6228-2tv9lb.csv" to "C:/Program Files (x86)/NREL/BEopt_3.0.0/./files/schedules20221212-6228-2tv9lb.csv"`

https://github.com/NREL/OpenStudio/blob/70df42e000cfe751e28a1e8c33b3c0c2e13786be/src/model/ExternalFile.cpp#L277

I can't tell if the problem is that it doesn't have permission or that there's a space in the path. Either way, I don't think there's a way to tell OpenStudio where to copy this file, so it'd be nice if it could more gracefully handle this.



Discussion / hints:
Why does OpenStudio have to make a copy of the schedule file anyway? Can't it just use the file directly for the EnergyPlus IDF? That would A) prevent issues like this and B) prevent having lots of extra files that aren't cleaned up when running lots of simulations.

Seeing if I can develop a workaround for BEopt...

@jmarrec @macumber @tijcolem @joseph-robertson 
@joseph-robertson and I determined that I can `Dir.chdir(...)` before calling `getExternalFile` to work around this issue.
I think the rationale was a model (with its companion directory) should be self contained so you don't run into issues if you simply move or modify a CSV file. After copying, the path is relative and relocating the model, such as moving back/forth a temp directory like OSApp does, just works.
I think this rationale is sound.
@shorowit what is schedules_path in your example? And what your cur directory and your model's path?
Trying to see if there is an actual bug here or just a feature request/suggestion.  Thanks
@jmarrec Looking back at this with fresh eyes, I think I really have a feature request.

My feature request is to add a `ScheduleFile` constructor that allows a path instead of an `ExternalFile` argument. Our residential software tools/workflows don't use the "companion directory" approach that the OS app does, and the use of an external file for ScheduleFiles creates more complications/headaches for us than anything. And it results in extra copies of CSV files in weird locations. Adding a second constructor would allow us to avoid the OS issue altogether, so we can just point the OSM directly to the CSV file path of our desire.

Is this feature request possible/practical?
I think that makes sense. I can look into whether that's pratical or not. If it is, we should clearly document that the user is responsible for ensuring the file exists and is valid though (a use at your own risk disclaimer basically)
Seems like we'd need to update the IDD. We'd need to either (a) have Schedule:File's "External File Name" field accept a `\type url`, or (b) introduce a new field, e.g. "File Path", that accepts a `\type url`. Something like that?
Also wouldn't we need an API break on `ExternalFile externalFile() const;` to `boost::optional<ExternalFile> externalFile() const;`?
Could you leave the IDD/API how it is now but, when a path is supplied to the `Schedule:File` overload constructor, automatically create an `ExternalFile` object behind the scenes that points to the specified path? So everything would work like it does now, in that the `Schedule:File` still points to an `ExternalFile`, but the `ExternalFile`'s File Name would point to the original path and not a copied location. That seems like it would minimize the changes needed.
So we'd need a new `ExternalFile` constructor (or use the existing ctor and introduce some kind of optional flag) that sets File Name as the full path, and also doesn't do all the parent folder / copy file stuff.
Sounds right to me.
Actually, maybe that's the better solution. If there was a new `ExternalFile` constructor (or existing ctor with optional flag), then you wouldn't have to make any changes to the `ScheduleFile` object. On our end, we could make use of it and then just continue to pass an `ExternalFile` object to the `ScheduleFile`. That's even less of a change on your end.
(I vote for the optional argument.)
I kind of like the idea of a `ScheduleFile` ctor where you pass in a path, though. Seems nice to not have to deal with another model object.

Fix patch:
diff --git a/src/model/ExternalFile.cpp b/src/model/ExternalFile.cpp
index ad1c74fe319..deed1eeb8ee 100644
--- a/src/model/ExternalFile.cpp
+++ b/src/model/ExternalFile.cpp
@@ -136,6 +136,9 @@ namespace model {
     path ExternalFile_Impl::filePath() const {
       path result;
       path fname = toPath(fileName());
+      if (openstudio::filesystem::is_regular_file(fname)) {
+        return fname;
+      }
       std::vector<path> absoluteFilePaths = this->model().workflowJSON().absoluteFilePaths();
       if (absoluteFilePaths.empty()) {
         result = this->model().workflowJSON().absoluteRootDir() / fname;
@@ -144,7 +147,7 @@ namespace model {
         // (eg goes to hardcoded paths, then potentially generated_files, then files, etc.)
         for (const openstudio::path& dirpath : absoluteFilePaths) {
           path p = dirpath / fname;
-          if (openstudio::filesystem::exists(p) || openstudio::filesystem::is_regular_file(p)) {
+          if (openstudio::filesystem::is_regular_file(p)) {
             result = p;
             break;
           }
@@ -206,12 +209,17 @@ namespace model {
 
   }  // namespace detail
 
-  boost::optional<ExternalFile> ExternalFile::getExternalFile(const Model& model, const std::string& filename) {
+  boost::optional<ExternalFile> ExternalFile::getExternalFile(const Model& model, const std::string& filename, bool copyFile) {
     path p = toPath(filename);
     if (!p.has_filename()) {
       return boost::none;
     }
-    std::string s = toString(p.filename());
+    std::string s;
+    if (copyFile) {
+      s = toString(p.filename());
+    } else {
+      s = filename;
+    }
 
     // this factory method will give us better control if we start doing things like copying files
     for (const auto& externalFile : model.getConcreteModelObjects<ExternalFile>()) {
@@ -223,13 +231,13 @@ namespace model {
     }
 
     try {
-      return ExternalFile(model, filename);
+      return ExternalFile(model, filename, copyFile);
     } catch (std::exception&) {
     }
     return boost::none;
   }
 
-  ExternalFile::ExternalFile(const Model& model, const std::string& filename) : ResourceObject(ExternalFile::iddObjectType(), model) {
+  ExternalFile::ExternalFile(const Model& model, const std::string& filename, bool copyFile) : ResourceObject(ExternalFile::iddObjectType(), model) {
     OS_ASSERT(getImpl<detail::ExternalFile_Impl>());
 
     WorkflowJSON workflow = model.workflowJSON();
@@ -246,40 +254,45 @@ namespace model {
     }
     OS_ASSERT(exists(p));
 
-    path destDir;
-    std::vector<path> absoluteFilePaths = workflow.absoluteFilePaths();
-    if (absoluteFilePaths.empty()) {
-      destDir = workflow.absoluteRootDir();
-    } else {
-      destDir = absoluteFilePaths[0];
-    }
-    path dest = destDir / p.filename();
-
-    if (exists(dest)) {
-      if (checksum(p) != checksum(dest)) {
-        this->remove();
-        LOG_AND_THROW("File \"" << p.filename() << "\" already exists in \"" << destDir << "\"");
+    bool ok;
+    if (copyFile) {
+      path destDir;
+      std::vector<path> absoluteFilePaths = workflow.absoluteFilePaths();
+      if (absoluteFilePaths.empty()) {
+        destDir = workflow.absoluteRootDir();
+      } else {
+        destDir = absoluteFilePaths[0];
       }
-    } else {
+      path dest = destDir / p.filename();
 
-      try {
-        makeParentFolder(dest, path(), true);
-      } catch (std::exception&) {
-        this->remove();
-        LOG_AND_THROW("Failed to created parent folder at \"" << dest << "\"");
-      }
-      try {
-        boost::filesystem::copy(p, dest);
-      } catch (std::exception&) {
-        this->remove();
-        LOG_AND_THROW("Failed to copy file from \"" << p << "\" to \"" << dest << "\"");
+      if (exists(dest)) {
+        if (checksum(p) != checksum(dest)) {
+          this->remove();
+          LOG_AND_THROW("File \"" << p.filename() << "\" already exists in \"" << destDir << "\"");
+        }
+      } else {
+
+        try {
+          makeParentFolder(dest, path(), true);
+        } catch (std::exception&) {
+          this->remove();
+          LOG_AND_THROW("Failed to created parent folder at \"" << dest << "\"");
+        }
+        try {
+          boost::filesystem::copy(p, dest);
+        } catch (std::exception&) {
+          this->remove();
+          LOG_AND_THROW("Failed to copy file from \"" << p << "\" to \"" << dest << "\"");
+        }
       }
-    }
-    OS_ASSERT(exists(dest));
+      OS_ASSERT(exists(dest));
 
-    bool ok;
-    ok = setFileName(toString(dest.filename()));
-    OS_ASSERT(ok);
+      ok = setFileName(toString(dest.filename()));
+      OS_ASSERT(ok);
+    } else {
+      ok = setFileName(filename);
+      OS_ASSERT(ok);
+    }
   }
 
   IddObjectType ExternalFile::iddObjectType() {
diff --git a/src/model/ExternalFile.hpp b/src/model/ExternalFile.hpp
index 09cd952b986..790e2d50345 100644
--- a/src/model/ExternalFile.hpp
+++ b/src/model/ExternalFile.hpp
@@ -69,7 +69,11 @@ namespace model {
 
     static std::vector<std::string> columnSeparatorValues();
 
-    static boost::optional<ExternalFile> getExternalFile(const Model& model, const std::string& filename);
+    /** This factory method will look for an existing ExternalFile with the same file name. If not it will try to create one.
+      * - If copyFile is true, the file will be copied to the first entry of the WorkflowJSON's absoluteFilePaths() (or absoluteRootDir() if empty);
+      * - If copyFile is false, it assumes this is an absolute path.
+      * In both cases, the filename should resolve to a valid path or this will return an empty optional. */
+    static boost::optional<ExternalFile> getExternalFile(const Model& model, const std::string& filename, bool copyFile = true);
 
     /** @name Getters */
     //@{
@@ -116,7 +120,7 @@ namespace model {
    private:
     REGISTER_LOGGER("openstudio.model.ExternalFile");
 
-    ExternalFile(const Model& model, const std::string& filename);
+    ExternalFile(const Model& model, const std::string& filename, bool copyFile = true);
 
     bool setFileName(const std::string& fileName);
   };
diff --git a/src/model/ScheduleFile.cpp b/src/model/ScheduleFile.cpp
index ba486118673..a5c0b66cd2a 100644
--- a/src/model/ScheduleFile.cpp
+++ b/src/model/ScheduleFile.cpp
@@ -412,6 +412,25 @@ namespace model {
     OS_ASSERT(ok);
   }
 
+  ScheduleFile::ScheduleFile(const Model& model, const openstudio::path& filePath, int column, int rowsToSkip)
+    : ScheduleInterval(ScheduleFile::iddObjectType(), model) {
+    OS_ASSERT(getImpl<detail::ScheduleFile_Impl>());
+
+    boost::optional<ExternalFile> externalfile = ExternalFile::getExternalFile(model, toString(filePath), false);
+    if (!externalfile) {
+      remove();
+      LOG_AND_THROW("Couldn't find the ExternalFile at " << filePath);
+    }
+
+    bool ok;
+    ok = setPointer(OS_Schedule_FileFields::ExternalFileName, externalfile->handle());
+    OS_ASSERT(ok);
+    ok = setColumnNumber(column);
+    OS_ASSERT(ok);
+    ok = setRowstoSkipatTop(rowsToSkip);
+    OS_ASSERT(ok);
+  }
+
   IddObjectType ScheduleFile::iddObjectType() {
     return {IddObjectType::OS_Schedule_File};
   }
diff --git a/src/model/ScheduleFile.hpp b/src/model/ScheduleFile.hpp
index ca8f0a17c52..d1e85d75691 100644
--- a/src/model/ScheduleFile.hpp
+++ b/src/model/ScheduleFile.hpp
@@ -58,6 +58,8 @@ namespace model {
 
     ScheduleFile(const ExternalFile& externalfile, int column = 1, int rowsToSkip = 0);
 
+    ScheduleFile(const Model& model, const openstudio::path& filePath, int column = 1, int rowsToSkip = 0);
+
     virtual ~ScheduleFile() = default;
     // Default the copy and move operators because the virtual dtor is explicit
     ScheduleFile(const ScheduleFile& other) = default;


Test patch:
diff --git a/src/model/test/ScheduleInterval_GTest.cpp b/src/model/test/ScheduleInterval_GTest.cpp
index ffa539b9174..7296634a1a0 100644
--- a/src/model/test/ScheduleInterval_GTest.cpp
+++ b/src/model/test/ScheduleInterval_GTest.cpp
@@ -380,3 +380,62 @@ TEST_F(ModelFixture, ScheduleFile) {
   EXPECT_TRUE(exists(p));
   EXPECT_FALSE(exists(filePath));
 }
+
+TEST_F(ModelFixture, ScheduleFileAltCtor) {
+  Model model;
+  EXPECT_EQ(0u, model.getConcreteModelObjects<ExternalFile>().size());
+  EXPECT_EQ(0u, model.getConcreteModelObjects<ScheduleFile>().size());
+
+  path p = resourcesPath() / toPath("model/schedulefile.csv");
+  EXPECT_TRUE(exists(p));
+
+  path expectedDestDir;
+  std::vector<path> absoluteFilePaths = model.workflowJSON().absoluteFilePaths();
+  if (absoluteFilePaths.empty()) {
+    expectedDestDir = model.workflowJSON().absoluteRootDir();
+  } else {
+    expectedDestDir = absoluteFilePaths[0];
+  }
+
+  if (exists(expectedDestDir)) {
+    removeDirectory(expectedDestDir);
+  }
+  ASSERT_FALSE(exists(expectedDestDir));
+
+  ScheduleFile schedule(model, openstudio::toString(p));
+  EXPECT_EQ(1u, model.getConcreteModelObjects<ScheduleFile>().size());
+  EXPECT_EQ(1u, model.getConcreteModelObjects<ExternalFile>().size());
+  ExternalFile externalfile = schedule.externalFile();
+  EXPECT_EQ(1u, externalfile.scheduleFiles().size());
+  EXPECT_EQ(openstudio::toString(p), externalfile.fileName());
+  //EXPECT_TRUE(externalfile.isColumnSeparatorDefaulted());
+  EXPECT_FALSE(equivalent(expectedDestDir / externalfile.fileName(), externalfile.filePath()));
+  EXPECT_TRUE(exists(externalfile.filePath()));
+  EXPECT_EQ(p, externalfile.filePath());
+  EXPECT_TRUE(schedule.isNumberofHoursofDataDefaulted());
+  EXPECT_EQ(1, schedule.columnNumber());
+  schedule.setColumnNumber(1);
+  EXPECT_EQ(1, schedule.columnNumber());
+  EXPECT_EQ(0, schedule.rowstoSkipatTop());
+  schedule.setRowstoSkipatTop(1);
+  EXPECT_EQ(1, schedule.rowstoSkipatTop());
+
+  EXPECT_EQ("Comma", schedule.columnSeparator());
+  EXPECT_TRUE(schedule.isColumnSeparatorDefaulted());
+  EXPECT_TRUE(schedule.setColumnSeparator("Tab"));
+  EXPECT_EQ("Tab", schedule.columnSeparator());
+
+  ScheduleFile schedule2(model, openstudio::toString(p));
+  EXPECT_EQ(2u, model.getConcreteModelObjects<ScheduleFile>().size());
+  EXPECT_EQ(1u, model.getConcreteModelObjects<ExternalFile>().size());
+  ExternalFile externalfile2 = schedule2.externalFile();
+  EXPECT_EQ(2u, externalfile.scheduleFiles().size());
+  EXPECT_EQ(externalfile.handle(), externalfile2.handle());
+  EXPECT_TRUE(schedule2.isNumberofHoursofDataDefaulted());
+  EXPECT_EQ(1, schedule2.columnNumber());
+  schedule2.setColumnNumber(2);
+  EXPECT_EQ(2, schedule2.columnNumber());
+  EXPECT_EQ(0, schedule2.rowstoSkipatTop());
+  schedule2.setRowstoSkipatTop(1);
+  EXPECT_EQ(1, schedule2.rowstoSkipatTop());
+}