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.

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

ExtModel cannot handle path formats from different OS #339

Closed
SCLaan opened this issue Sep 20, 2022 · 9 comments · Fixed by #361
Closed

ExtModel cannot handle path formats from different OS #339

SCLaan opened this issue Sep 20, 2022 · 9 comments · Fixed by #361
Assignees
Labels
type: feature Brand new functionality
Milestone

Comments

@SCLaan
Copy link

SCLaan commented Sep 20, 2022

Is your feature request related to a problem? Please describe.
When working on Windows ExtModel (and maybe other functions) cannot read Unix paths (and probably vice-versa). With many quick pre- and post-processing steps on Windows machines and the larger simulations on a Linux cluster an automatic conversion of path formats when reading data will be necessary. Also for saving it will be a good feature to be able to choose the output format (Windows/Unix).

To Reproduce

  1. Make an external forcings file (dummy.ext) with the content:
[boundary]
quantity=waterlevelbnd
locationfile=/p/11203669-004-kpp-morfwad-krw-slib/Waddenzee_model_hydro/04_Boundary_conditions/east_v05_part2.pli
forcingfile=/p/11203669-004-kpp-morfwad-krw-slib/Waddenzee_model_hydro/04_Boundary_conditions/east_part2_wl_20151222_20180101.bc
return_time=0.0000000e+000
  1. Open external forcings file in python on Windows:
from pathlib import Path
from hydrolib.core.io.ext.models import ExtModel
extFile = Path(r'dummy.ext')
extData = ExtModel(extFile)
  1. This causes an error like:
DWSM-FM_bc.ext -> boundary -> 19 -> forcingFile
  File: `\\directory.intra\Project\p\11203669-004-kpp-morfwad-krw-slib\Waddenzee_model_hydro\04_Boundary_conditions\south_temp_20151222_20180101.bc` not found, skipped parsing. (type=value_error)

Describe the solution you'd like

  • ExtModel to automatically determine the format of paths and to be be able to read the data regardless of the used OS.
  • ExtModel to include an option for saving in a different format (e.g. save as Unix on Windows). Ideally this is set based on the format already used.

Technical design
(Proposal by @arthurvd)

  • Implementation in the FileModel class is probably enough and as high as possible.
  • It should support optional configuration of the path-style (Windows or Unix-like) during model load()
  • A similar optional argument should support a custom path-style upon save() of a model
  • By default, the current OS style must be detected and used.
  • Path style options can become a small Enum class.
  • Probably this must become a data attribute in the FileModel class, such that it can also be set while creating models from scratch, e.g.:
fmmodel = FMModel()
extmodel = ExtModel()
extmodel.filepath = '/p/foo/forcing.ext`
extmodel.path_style = PathStyle.UNIXLIKE   # or do we even autodetect this in set_attribute or so?
  • For absolute paths, no translation is possible (we don't know which Windows driveletter corresponds with which leading root dir on Linux). OR: should the user be able to supply some mapping table of absolute path parts?
  • Final remark: before making all of this: INVESTIGATE whether pathlib or some other package already has some built-in features for this.
@arthurvd arthurvd added the type: feature Brand new functionality label Sep 20, 2022
@arthurvd arthurvd added this to the Release 0.4 milestone Sep 20, 2022
@veenstrajelmer
Copy link
Collaborator

Suggestion: always write forward slashes (windows also supports this), then the only issue that remains are the hardcoded drive letters

priscavdsluis added a commit that referenced this issue Oct 10, 2022
priscavdsluis added a commit that referenced this issue Oct 10, 2022
priscavdsluis added a commit that referenced this issue Oct 10, 2022
@priscavdsluis
Copy link
Contributor

I put this is back in TODO for now, since I would like to discuss this with the rest of the team:

  • Resolving absolute paths from posix to windows will indeed be a challenge. For example, in this, case /p has to be converted to p:. This in itself is not difficult, but it will become trial and error code: if the converted path can be found, it was apparently an absolute path, otherwise it must be a relative path. From windows to posix, will be easier.
  • I agree with @veenstrajelmer, that we could always write forward slashes. This way, all relative paths that were written with hydrolib-core can be correctly read with hydrolib-core on any OS. However, what about models that already exist with backward slashes?

@priscavdsluis
Copy link
Contributor

@SCLaan, thanks for your feedback! Good to hear it works.

@arthurvd
Copy link
Member

@SCLaan : wil jij deze nieuwe functionaliteit testen?
Het staat in @priscavdsluis's branch #361 (feat/339-handle-path-format-different-OS), en je gebruikt het bijvoorbeeld zo (op Windows verwijzend naar files met Linux /p/enzovoort paden.

inputmdu=Path(r"p:/model/flowfm.mdu")
fmmodel = FMModel(filepath=inputmdu, path_style="unix")

# [..]
outputmdu=Path(r"p:/model/flowfm_modified.mdu")
fmmodel.filepath = outputmdu
fmmodel.save(path_style="unix")

@priscavdsluis
Copy link
Contributor

#474 can be merged once this issue is merged (should make tests green for that issue)

priscavdsluis added a commit that referenced this issue Mar 28, 2023
Co-authored-by: Arthur van Dam <arthurvd@gmail.com>
@SCLaan
Copy link
Author

SCLaan commented Mar 28, 2023

@SCLaan : wil jij deze nieuwe functionaliteit testen? Het staat in @priscavdsluis's branch #361 (feat/339-handle-path-format-different-OS), en je gebruikt het bijvoorbeeld zo (op Windows verwijzend naar files met Linux /p/enzovoort paden.

inputmdu=Path(r"p:/model/flowfm.mdu")
fmmodel = FMModel(filepath=inputmdu, path_style="unix")

# [..]
outputmdu=Path(r"p:/model/flowfm_modified.mdu")
fmmodel.filepath = outputmdu
fmmodel.save(path_style="unix")

I did some quick tests and this seems to be working fine. I think this issue can now be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature Brand new functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants