Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Small refactoring CR23 - File deletion #1732

Merged
merged 4 commits into from
Nov 2, 2023
Merged

Small refactoring CR23 - File deletion #1732

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1984b81
CR 23 Refactoring : Removing deletion of deprecated files from Load f…
nikolaredstork Oct 31, 2023
ca51b02
Excluding unnecessary bool variable
nikolaredstork Oct 31, 2023
76d242e
Build error
nikolaredstork Oct 31, 2023
ea6c725
Excluding automatic file removal
nikolaredstork Nov 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions src/libs/antares/study/area/list.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -934,14 +934,6 @@ static bool AreaListLoadFromFolderSingleArea(Study& study,
study.parameters.derated, study.header.version, study.usedByTheSolver);
}

buffer.clear() << study.folderInput << SEP << "hydro" << SEP << "common" << SEP
<< "capacity" << SEP << "maxpower_" << area.id << ".txt";

if (bool exists = IO::File::Exists(buffer); study.header.version >= 870 && exists)
{
IO::File::Delete(buffer);
}

++options.progressTicks;
options.pushProgressLogs();
}
Expand Down
15 changes: 15 additions & 0 deletions src/libs/antares/study/study.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1542,5 +1542,20 @@ void Study::computePThetaInfForThermalClusters() const
}
}

void Study::deleteDeprecatedFiles(uint previousVersion)
{
if (previousVersion < 870)
{
const std::string filePath(folderInput + "/hydro/common/capacity/");
areas.each(
[&filePath](const Area& area)
{
const std::string fileName(filePath + "maxpower_" + area.id + ".txt");
if (bool exist = IO::File::Exists(fileName); exist)
IO::File::Delete(fileName);
});
}
}

} // namespace Antares::Data

6 changes: 6 additions & 0 deletions src/libs/antares/study/study.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,12 @@ class Study: public Yuni::NonCopyable<Study>, public IObject, public LayerData
void removeTimeseriesIfTSGeneratorEnabled();
//@}

/*!
Copy link
Contributor

@guilpier-code guilpier-code Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Class Study is large enough, we plan to split it in many pieces, and we don't want it to grow.

  • For now, we should make this function a free function. We could add it as a couple of source files file-removal.h/.cpp
  • What it actually does is removing deprecated hydro max power input files, right ? Don't hesitate renaming it removeDeprecatedHydroMaxPowerTSfiles.
  • this function should take 3 arguments :
    • a list of areas (AreaList&),
    • a input folder path (as an std::string or in a standard format, we don't want dependencies on Yuni strings, we want to get rid of it)
    • a current study version (as a unsigned int)

It could be something like :

void removeDeprecatedHydroMaxPowerTSfiles(AreaList& areas, 
                                          std::string inputFolder, 
                                           unsigned int currentStudyVersion)
{
    if (currentStudyVersion >= 870)
        return;
    
    vector<std::string> areasNames;
    areas.each( [](const Area& area) { areasNames.push_back(area.id.to<std::string>(); });
    
    for (const name :  areasNames)
    {
        std::string filePath = folderInput + "/hydro/common/capacity/" + name + ".txt"
        removeFile(filePath); // To be coded in the same source file, in the .cpp file.
    }
}

On the client side, it would be :

removeDeprecatedHydroMaxPowerTSfiles(study.areas, study.folderInput.to<std::string>, study.header.version),

Note : about removeFile(filePath), it probably already exists in the unit tests code area

Copy link
Collaborator Author

@nikolaredstork nikolaredstork Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to first read currentStudyVerstion and save it value to temporary variable, after that we are calling study->saveToFolder. Why we need temporary variable? We need it because, if we are upgrading a study, variable study->.header.version will be changed (after study->saveToFolder function call) to a current version of our software, and if we are passing value of the study->.header.version variable to the deleteDeprecatedHydroMaxPowerTSFiles function we will be passing already updated value of our study version (currently 870). If we are upgrading for example from 8.2 version than we need to pass 820 value to the deleteDeprecatedHydroMaxPowerTSFiles function, and we are doing that via temporary variable.
Condition if (currentStudyVersion >= 870) must be changed to if(previousVersion < 870) . Why? Because currentStudyVersion after saving a study will have 870 value, and deletion won't be performed.

Copy link
Collaborator Author

@nikolaredstork nikolaredstork Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we need to delete files just if study->saveToFolder() function call return true, than we are sure that our new files are saved and all data from deprecated files are stored in our new files.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. Your last comments regard more the updater/main.cpp than the src/libs/antares/study/study.h.
So I answer you in updater/main.cpp (below)

** \brief Delete deprecated files when upgrading a study
*/
void deleteDeprecatedFiles(uint previousVersion);
//@}

//! \name
//@{
/*!
Expand Down
7 changes: 6 additions & 1 deletion src/tools/updater/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,12 @@ class MyStudyFinder final : public Data::StudyFinder
study->parameters.readonly = true;

logs.info() << "Saving...";
study->saveToFolder(folder);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally, we should add this to the src/libs/antares/study/cleaner/cleaner.cpp, which is supposed to clean any study from unsuitable files.

Copy link
Collaborator Author

@nikolaredstork nikolaredstork Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I've looked up cleaner.cpp file in details, and apparently after upgrading and saving our new files, we will delete deprecated files with clean command, so I don't think that we need automatic deletion after upgrade. With that been said, this PR shouldn't be merged and code for deletion of files should be excluded.

uint tempVersionVar = (uint)study->header.version;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tempVersionVar : we should be more clear. This variable is the current study version, isn't it ?
So let's call it currentStudyVersion

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An answer to the previous comment from @nikolaredstork was given here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is what think, eventually about tempVersionVar and this added piece of code.
I think we should have :

  ...
  bool isSaved = study->saveToFolder(folder);
  if (isSaved)
      deleteDeprecatedHydroMaxPowerTSFiles(study->areas, 
                                           study->folderInput.to<std::string>, 
                                           study->header.version);

Note we don't need the source (what you called tmp) study version here.
Why ?
When get here, a study was loaded, and it may have input deprecated files.
Then we upgrade it, meaning that we erase input data with fresh updated one.
Deprecated files (hydro max power TS in this case) can still remain.
Calling deleteDeprecatedHydroMaxPowerTSFiles will delete them if they exists and if the new study version requires it.
Note that the small removeFile function suggested in this comment removes a file only if it exists of course.

It's more simple like this, don't you think ?

bool isSaved = study->saveToFolder(folder);

if (isSaved && (tempVersionVar != Antares::Data::versionLatest))
study->deleteDeprecatedFiles(tempVersionVar);
}
else
{
Expand Down
6 changes: 5 additions & 1 deletion src/ui/simulator/application/study.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,11 @@ class JobSaveStudy final : public Toolbox::Jobs::Job
} // save as

Copy link
Collaborator Author

@nikolaredstork nikolaredstork Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guilpier-code This is obviously code duplication. Do you have proposal where potential function should be defined?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guilpier-code This is obviously code duplication. Do you have proposal where potential function should be defined?

As you know, we don't want to enhance the UI code, we think it's not worth it.
Besides, it's not exactly code we touched for this CR 23.
But you're right, there is a code duplication.

// Save the study (only changes in the most cases)
study->saveToFolder(pFolder);
uint tempVersionVar = study->header.version;
bool isSaved = study->saveToFolder(pFolder);

if (isSaved && (tempVersionVar != Antares::Data::versionLatest))
study->deleteDeprecatedFiles(tempVersionVar);

// Scenario Builder
if (pCanReleaseScenarioBuilder && study->scenarioRules)
Expand Down