Option to load model as a copy (read only mode)

[TermeHansen]

I would like to have this option to be able to load the model in safely as a copy (read only mode). I suggested this earlier ( https://github.com/MPh-py/MPh/pull/82), but I got caught up in other work so we didn't get it in at that time - sorry!

Now suggesting it again but as a very minimal change :)

[john-hen]

Yeah, that's fine, we can add that.

The option should be explained in the doc-string of that method.

What's the expected behavior when you call model.save() on a model loaded in this way? I.e., in read-only mode. Have you tested/considered that?

[TermeHansen]

There seem to be no check of this kind from the java side of Comsol, so one can actually fine save the model even though it is opened as a copy.

Not sure if we want to to extend this with also handling the save function for this special casing. I have pushed a commit with suggestions to what could be done. I have tested the different cases and it works as intended.

I am not sure if the lock file check will break things for some people, but this is actually the core of this whole thing for me. If the model file is also opened by Comsol at the same time I have at several occasions ended up in a state where Comsol will not save the file and it has in the same process deleted the original file - leaving me with NOTHING :-O

[john-hen]

There seem to be no check of this kind from the java side of Comsol, so one can actually fine save the model even though it is opened as a copy.

So calling loadCopy() instead of load() does not actually achieve what you want.

I have tested the different cases and it works as intended.

We should talk more about the intent of the feature, as that informs the implementation.

The documentation of loadCopy() says:

Load a copy of a model. To save the model a new filename much [sic] be provided.

So this has nothing to do with "read-only mode" per se. It just means that the Comsol model object (that MPh's Model object is wrapped around) does not keep track of the file that the model was loaded from. I would suspect that model.java.getFilePath() (which we call in model.file()) then returns an empty string or something. And when you try to save that model without providing a file name, i.e. with just model.save(), it should fail, because it has no target to save to. (I haven't tested that, just extrapolating from the documentation. I probably ignored loadCopy() in the first place because I didn't really see the point of that behavior. And it just complicates things if the file path could possibly be undefined.)

As I understand it, what you really want is have the .save() method respect the "write lock", as indicated by the presence of the .lock file. But then we don't need to load a "copy". To check for the lock, you need a file path anyway. So why not hold on to this information when the model is loaded?

This change should not, in any way, break backwards compatibility. This is important to me. Even if the old behavior is arguably bad, it would have to be deprecated gradually, with a user warning or something, for a reasonable amount of time (in terms of new releases in between).

According to your tests then, the Comsol API methods also don't respect the write lock. Only the Comsol GUI does. This is consistent with how I think about the problem. Because I feel like this check should be made in application code, not in the library code. But we can of course make things easier for end users. Like by providing the model.locked() method that you suggest. And maybe an optional argument to model.save(), like respect_lock=False, that defaults to the old behavior — for now, at least, if not indefinitely.

Though I think model.locked() is enough. You'd just have to do something like this in application code:

if not model.locked():
    model.save()
else:
    raise PermissionError(f'Model file "{model.file()}" is locked by another process.')

Even if we added a check in model.save(), it couldn't do more than raise that error. As opposed to application code, where you might be able to handle that case differently. Like by saving the model under a different file name of your choosing.

[john-hen]

I think it makes sense to add the Model.locked() method. It would be a welcome addition to the API. It should come with a doc-string that explains why the feature exists. (To avoid clashes with the Comsol GUI when saving the model. Elaborating on that problem is the value added.) And a test should be added to the test suite accordingly. (Like, just create the .lock file and make sure Model.save() then fails, when asked to respect the lock.)

But then it may just be a new pull request. So closing this one.