microscope-cockpit / cockpit

Cockpit is a microscope graphical user interface. It is a flexible and easy to extend platform aimed at life scientists using bespoke microscopes.
https://microscope-cockpit.org
GNU General Public License v3.0
37 stars 27 forks source link

Save path customisation #791

Open thomasmfish opened 2 years ago

thomasmfish commented 2 years ago

Further to the now closed #786, the current save path methods aren't suitable for use at B24 as it doesn't offer enough flexibility. At Diamond we store data with the following structure:

remote-directory/data/<year>/<visit id>/raw/

Within which a user may specify their own structure. A visit ID is known by the user but is not stored on the PC, so a user would have to navigate there themselves. As such it'd be useful to have a generic directory to begin the navigation from, without that requiring a subdirectory to be created (no-one has permissions to write there anyway, so this causes an error). We also don't want the username to be a subdirectory.

What would be the best way to make this possible without breaking existing implementations?

iandobbie commented 2 years ago

I guess we need some macro expansion in the file name, but we should be carful that we don't introduce horrid hard ti track down bugs. In on way I would encourage the use of the shell and shell variables but I also remember all the horrid security issue this has thrown up down the years.

Is there any easy programatic way to get at the visit id? We could synthesize this at config startup.

thomasmfish commented 2 years ago

No, there's nothing we can do to reliably get the correct visit id. I've made some changes in a branch that I think is overall a nicer way to handle things:

  1. No longer check & create a data directory during start up
  2. Allow more configured flexibility (substitute in {user}, {year} etc)
  3. Check the validity of the base data directory when data is actually going to be written to it (allows the directory to be manually changed then, rather than preventing Cockpit from starting!). This wasn't done before, so experiments would fail to start without properly reporting the issue.

That said, there is a significant issue with my changes as it removes the automatic addition of the username to the data directory, which would require the addition of \{user} at the end of the configured path to keep the same behaviour. While I see this as overall an improvement, it could cause issues when people update.

thomasmfish commented 2 years ago

Had some issues with checking networked write access from a Windows PC. The best option, from what I can see, is to try and write a file, then clear it up (I wish I was joking).

Regardless of whether you want all the changes, there should be more checks in the GetPath function (since the directory can be manually set), so I think the branch I linked has value either way.

thomasmfish commented 2 years ago

I thought I should delete the PR and link the branch with the changes. It'll need rebasing/merging with master before testing

carandraug commented 5 months ago

I was looking into this and noticed that data-dir already expands variable environments. If you managed to inject the year and visit-id on the user environment, you can simply set data-dir to remote-directory/data/${YEAR}/${VISIT_ID}/raw/.

thomasmfish commented 5 months ago

Unfortunately, this isn't something we can set in environment variables, so, the experiment directory needs to be defined after starting the software (we don't want users routing around in the config!). The key requirements for us are:

  1. No directory created at start
  2. No username subdirectory

The ability to use some common variable substitutions seemed like a way to have OS-independent behaviour, and for us it gives a reasonable place for users to start navigating from. The way I've handled those variables in substitute_patterns should be easy to extend to other variables if people want that too.

carandraug commented 5 months ago

Unfortunately, this isn't something we can set in environment variables

Could you replace the cockpit executable with another that first asks the user to enter the visit-id, set that env variable, and then start the "original" cockpit executable?

The key requirements for us are:

  1. No directory created at start

If you are relying on the user setting it later and you just don't want the directory to be created at the start, set the directory to something that definitely exists and the user won't have write permissions, such as / (or C:\ on windows).

But I agree with your earlier comment that it would be nice for cockpit to handle better the case of data-dir not existing. The rest of cockpit should not assume that data-dir exists --- just because it exists at the start does not mean it will still exist during an experiment 2 hours later. But I'm not sure what to do (ask to create all directories, ask for alternative location, error, in all cases?). If we have a good behaviour for that, then it makes sense to think about removing the creation of data-dir at start.

  1. No username subdirectory

This is now the default (since 6b4d3bc0). Places/People that want username subdirectory can use variable expansion (both windows and Unix have default env variables for username).

The ability to use some common variable substitutions seemed like a way to have OS-independent behaviour, and for us it gives a reasonable place for users to start navigating from. The way I've handled those variables in substitute_patterns should be easy to extend to other variables if people want that too.

We should never require people to modify the code.

Anyway, we already have variable substitution, it's just that the variables are read from the environment. If you can't set env variables when a user logins, are you able to set those variables during runtime? I'm thinking two options (not sure the details of how on windows):

thomasmfish commented 5 months ago

If you are relying on the user setting it later and you just don't want the directory to be created at the start, set the directory to something that definitely exists and the user won't have write permissions, such as / (or C:\ on windows).

We typically do but mapping network drives means we cannot necessarily rely on it always being the same. I want to start users in a good place that is close to their data, so they don't have to navigate through the maze of directories we have, not in C:\, which limits things. Also, some people can write in C:\, so that would get messy.

But I agree with your earlier comment that it would be nice for cockpit to handle better the case of data-dir not existing. The rest of cockpit should not assume that data-dir exists --- just because it exists at the start does not mean it will still exist during an experiment 2 hours later. But I'm not sure what to do (ask to create all directories, ask for alternative location, error, in all cases?). If we have a good behaviour for that, then it makes sense to think about removing the creation of data-dir at start.

On the version we have running it handles checking whether the directory exists and is writeable, so I think those changes from this branch could be useful for that.

This is now the default (since https://github.com/microscope-cockpit/cockpit/commit/6b4d3bc06ea9454ed035835c2c41a49ea0f5abc4). Places/People that want username subdirectory can use variable expansion (both windows and Unix have default env variables for username).

I wasn't aware of that change! Good to know, thanks.

We should never require people to modify the code.

I absolutely wasn't suggesting people would be required to modify the code! All I meant is that I had included a method that would be easy to modify, should other substitutions be requested.

Anyway, we already have variable substitution, it's just that the variables are read from the environment. If you can't set env variables when a user logins, are you able to set those variables during runtime? I'm thinking two options (not sure the details of how on windows):

  • launcher/icon/shortcut for cockpit set that env variable first (something like COCKPIT_YEAR=$(date '+%Y') cockpit and something similar may be possible on windows according to https://superuser.com/a/424002 )
  • replace the cockpit executable with a simple shell script (or the windows equivalent) with that

It should be possible to do something like that but having to play with environment variables and modify the launcher feels pretty clunky and not the most accessible solution. I wouldn't be surprised is other people also would find using date variables useful substitutions when configuring the paths.

Windows is annoying... %DATE:~6,4% does get the year in Windows but only if you have the date format set a certain way because it gets characters 7-10 from the date string, which may be an issue if users change their preferences.

Personally, I think I'd prefer to set it early in the Cockpit initialisation using Python, if I have to use environment variables, or copy my existing solution over when I next update things - no reason both forms of subsitution can't exist.