Check work_dir with project check CLI and enforce absolute paths

[ml-evs]

Continuation of my last PR, this closes #14 and also adds a task for checking the configured work_dir (c.f. #13). I fell into various pits trying to set mine up (using e.g., ~, $HOME or relative dirs did not work, so I changed the config description to force absolute paths).

[ml-evs]

This was the simplest place to add/"fix" this behavior, as the configured QResources fields need to be mapped before the submission script is created (see #14)

[gpetretto]

In principle it picks the resources from the worker at runtime:

jobflow-remote/src/jobflow_remote/jobs/runner.py

Line 382 in 808ca4a

resources = fw_job_data.task.get("resources") or worker.resources or {}

Did this not work?
I don't have a strong opinion whether it is better to fix it at creation time or run time, but I would keep only one point where it is set, otherwise it might be confusing.

[ml-evs]

My bad, after checking again with the new slurm fields, it does indeed work at runtime without this change. I've undone these changes

[gpetretto]

Thanks for the implementation.

I have some questions/concerns about this:

• Does switching from str to Path as a type for work_dir help in determining the fact that path should be absolute? Does this allow to use elements like ~?
• To enforce the fact that the path should be absolute the easier and safer way would probably be to add a validator in the model.
• In the _check_workdir maybe the file could be deleted after the test?
• I don't really want to enforce the use of the QResources in the config file at this stage for multiple reasons. Mainly because it is basically untested and I don't think it will be good that the only way the users have to pass the configuration is to learn our own naming convention, without even the guarantee that will continue to exist in the future. QResources also does not cover all the possible cases. I also think that passing slurm/pbs keywords will be a request from the customers at this stage. As far as I am concerned resources should either be simply a dict and eventually be converted to Qresources at runtime, depending on its content, or being a dict | QResources. In both cases it will probably be a bit annoying to handle and the reason I did not do it in the first place.

[ml-evs]

Thanks for the implementation.

I have some questions/concerns about this:

* Does switching from `str` to `Path` as a type for `work_dir` help in determining the fact that path should be absolute? Does this allow to use elements like `~`?

No, but it seemed like the more appropriate type hint. It will still be stored as e.g., Path(~/blah) with str(Path("~/blah")) == "~/blah" so current behavior should not change. It does not seem like the Fabric connection does any shell/user expansion at the moment.

* To enforce the fact that the path should be absolute the easier and safer way would probably be to add a validator in the model.

Yep, was going to ask if you wanted this.

* In the `_check_workdir` maybe the file could be deleted after the test?

👍

* I don't really want to enforce the use of the QResources in the config file at this stage for multiple reasons. Mainly because it is basically untested and I don't think it will be good that the only way the users have to pass the configuration is to learn our own naming convention, without even the guarantee that will continue to exist in the future. QResources also does not cover all the possible cases. I also think that passing slurm/pbs keywords will be a request from the customers at this stage. As far as I am concerned `resources` should either be simply a `dict` and eventually be converted to Qresources at runtime, depending on its content, or being a `dict | QResources`. In both cases it will probably be a bit annoying to handle and the reason I did not do it in the first place.

Fair enough -- if the expected behaviour is to pass queue-specific stuff in this config, then I can change that back to a raw dict. I think the way it is currently laid out, it will still need the other change that passes the config resources into the flow converter function, but I can verify this first.

[ml-evs]

I've just rebased this PR to remove the initial concern in #14 and instead just contains a few tweaks. Feel free to do what you want with it!

[gpetretto]

Thanks a lot for all the updates.