MPh-py / MPh

Pythonic scripting interface for Comsol Multiphysics
https://mph.readthedocs.io
MIT License
274 stars 70 forks source link

Classkit license support #40

Closed max3-2 closed 3 years ago

max3-2 commented 3 years ago

I opened this PR to stop cluttering chat - again my solution is obviously WiP. The initial discussion pulled from chat is

On another note: You can take a look at …

There's a special student "classkit" license. The server call accepts -ckl to choose this license It will not work with standalone since there seems to be no option to start that using the option (at least i found nothing in > the docs) COMSOL is dumb and not aware of available licenses, thus not using -ckl with only classkit licenses available yields a licence error on client.load() as does the way around, e.g. using classkit with a real license

I currently have someone running from this commit and will report back if it works.

Regarding your remarks:

a Comsol installation on Windows would have a file named …

Sadly, can not confirm. Neither for classkit nor for full installations. I wonder what’s the difference anyway. I can change license server and shortcut and then it works on windows. On Unix, I cannot really see a difference since there is not real shortcut so switching back and forth is possible. Also, with a classkit installation, the exe only has "C:\Program Files\COMSOL\COMSOL54\Multiphysics\bin\win64\comsol.exe" -Dcs.ckl, missing the additional line as stated in the docs. I also checked that against the 5.4 and 5.5 Installation guide - same story. So from the installation we have here, using the file does not seem to be an option. I don’t know if this has to do with the centralized license server build, however I do not see this being a stable solution and the central license server should be quite common in larger ecosystems.

Also, for Windows it says the command-line argument is -Dcs.ckl

True. It also seems to accept -ckl. My experience with java is too little to follow you why you would assume that the above is a java argument. If it is we should enable stand-alone with the argument above on windows. I can test that here.

john-hen commented 3 years ago

This should be the first priority right now. I see you already made some simplifications compared to the first draft. It's nice and clean now. I'd rather call the option just "classkit" though, because it's shorter.

So this means users with a classkit license will have to call mph.option() in application code. Obviously it would be nicer if that just worked out of the box and without any code changes. But if we have no way of identifying an installation that needs to use the classkit license, then that's the way to go.

-Dcs.ckl looks like a JVM argument because these are often prefixed with -D to distinguish them from arguments to the actual Java program that the JVM is running. It's just a convention though, could be a coincidence. Maybe Comsol developers just wanted to avoid similar conflicts. But if it works anything like -Djava.library.path, which I know jpype.startJVM accepts, then we could try something similar and set the cs.ckl property to true (in Java). (See also this test script which calls System.getProperty to read a JVM property.)

I could write a test script for that. (Not sure when and definitely not today.) But can you test stand-alone mode? I guess the test would simply be if the model can be loaded, as per your explanation above.

max3-2 commented 3 years ago

At least on windows i can test that easily. If you give me a snippet on how to set the option I can try a proof of concept. Or do you just go with ‘System.setProperty‘?

This will be interesting since this more seems to be a server option. Don’t know if the stand alone client accepts it…

john-hen commented 3 years ago

System.setProperty may not work because it's not writeable. Or maybe that depends on the property. I don't remember all the details. I played around with that a few month back when I tried to figure out the issue with stand-alone mode on Linux. There is a way to set properties, but I think one has to create a copy of... something, change the copy, and then like "paste" back the entire thing. Along those lines.

But it could also be much simpler. Either because cs.ckl is not in the java.* name space and we can use System.setProperty, or because passing -Dcs.ckl to jpype.startJVM just works.

john-hen commented 3 years ago

So passing -Dcs.ckl works? Did you test that already?

max3-2 commented 3 years ago

I spend the good part of this morning testing getting license errors on and on just to realize that there seems to be a license issue deeper in our system since even the GUI won't start. Sometimes it's just bad luck. I will fix that first and we will see. I still think the core issue will persist since it was reported from a different machine. If this is false alarm, even better...

At least I learned that you can you the args list to pass args to jvm and also the jvm is picky, e.g. raises exceptions if the args are not recognized at all.

max3-2 commented 3 years ago

Ok I worked it out on my side and can confirm that the current commit works on windows and ubuntu with either stand-alone (windows only) and server-client.

Additionally, I realized during debugging that mph.option() does not check if the option is available, e.g. the user can add arbitrary options.

This makes tracking down typos quite hard....

john-hen commented 3 years ago

No, I don't think it should be in the demo scripts. They should be as simple as possible. If anything, we should consider adding a way of setting config options via a configuration file, like MPh.ini in the user profile. Just so users don't have to set the license in code. Would have to be platform-independent. So takes a little bit of thought. Not in this PR.

I don't know how to test this either. That's fine.

Yeah, mph.option() could do some validation of the arguments. The whole reason the interface is a function, and not just a module, is that there can be validation. It's just missing so far.

john-hen commented 3 years ago

I tried some editing during the review. Not sure I did this right or if there's a better way to do it.

max3-2 commented 3 years ago

Don’t know either but it works for me. From my side this could be merged after the try except is removed. If you want and have the time you should have write Access to the PR branch. I would move the try except to another commit and probably add the options checks for 1.0.4. Maybe add the error verbosity from the other issue too. This would be a nice bugfix release plus a new ‚feature‘ with ckl.

While jpype has merged the PRs I think a release can easily take another few days (weeks?)

john-hen commented 3 years ago

Yeah, no worries, I'll take it from here and will try to get 1.0.4 out on Sunday so the classkit works a.s.a.p. I don't have a lot of time this weekend, but that should be doable. I was hoping the new JPype would be ready by now, but apparently there are some lingering issues with Python 3.10 that need fixing. So there's probably gonna be 1.0.5 as well.

max3-2 commented 3 years ago

Great thank you. I’m on vacation until next week and also have finished the current task where I was using mph myself (including a first? Cite if it will be accepted…). As soon as the new version is out I will inform the people at my institute so they can continue to work and debug 😊

john-hen commented 3 years ago

I added support for loading the configuration from a file, MPh.ini. See the documentation of the config module. I hope it works as intended. Enjoy your vacation.