MPh-py / MPh

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

Missing support for Apple Silicon architecture on M1/M2 Mac #80

Open ehsanhaghighat opened 2 years ago

ehsanhaghighat commented 2 years ago

Thanks for the tool - I found that the import doesn't handle COMSOL6/macarm64.

john-hen commented 2 years ago

Hi, thanks for bringing this up. I don't have access to an M1 Mac, so can't test any of this. The fix you're suggesting in the title, do you mean to replace any occurrence of "maci64" with "macarm64" in discovery.py?

Also, I suppose you used the "universal2" Python installers for macOS, so this is running natively on Apple Silicon, not through Rosetta. And when you installed Comsol 6, could you choose between the two variants? In other words, is there a chance Comsol could either run natively or through the emulation layer on the M1 Mac? Probably shouldn't matter for the discovery mechanism, just trying to get a feel if there's a way we might break backward compatibility.

doub1emint commented 2 years ago

I have the same problem, and I replace any occurrence of "maci64" with "macarm64" in discovery.py. it worked! thanks for effective suggestions.

max3-2 commented 2 years ago

In other words, is there a chance Comsol could either run natively or through the emulation layer on the M1 Mac? Probably shouldn't matter for the discovery mechanism, just trying to get a feel if there's a way we might break backward compatibility.

COMSOL states on their site for Apple Sillicon that is is a good practice to

Generally, install both the native (M1) and Intel versions of COMSOL and use the version that suits you best.

So I assume that you can install both in parallel and the discovery will show both, as soon as the second search would be implemented. How (if) is a priorization implemented if not on Version number? So what would be the default on macs with both options installed?

I sadly do not have unconditional access to COMSOL anymore so I can just assume some things. Still waiting on a project where I can justify using it

john-hen commented 2 years ago

Ah, okay. That's a good hint, was not aware of that. And yes, that is a complication because it's not clear which one to choose. Would make sense to select depending on which Python is running, but I don't know exactly (yet) how to do that. I'm starting a new job soon. Won't be working with Comsol, but will have a Mac M1 as my developer machine, and perhaps (not guaranteed) access to a Comsol campus license. So will be looking into it then, though in due time obviously. Also, if both can be installed and is even recommended, it should actually work, I would assume, if that recommendation is followed. Though obviously, it would be preferable to use the Apple Silicon variant.

john-hen commented 2 years ago

So, turns out, my "new Mac" isn't all that new, it's still Intel-based. I can test on that, but not on Apple Silicon. But I do have a campus license, including the latest Comsol version. If anyone wants to submit a PR that handles both system architectures on a Mac, I could help out with testing, with that limitation in mind. Otherwise it's unlikely I'll fix this issue myself anytime soon.

silveira-lucas commented 2 years ago

I have the same problem, and I replace any occurrence of "maci64" with "macarm64" in discovery.py. it worked! thanks for effective suggestions.

Hi doub1emint, I tried your suggestion without success. I changed every instance of the word "maci64" for "macarm64", even in the comments. I also searched for it in the other package files. However, strange enough, I still the error message which mentioning "maci64". I appreciate any help, please pleaaaase :)

doub1emint commented 2 years ago

Hi, silveira, I'm sorry to hear that and I have no idea about your problem.

pnorgaard commented 1 year ago

I'm running on an Apple M1 Pro, OS 13.4.1, and found that doub1emint's suggestion worked for me. Looks like "maci64" doesn't need to be replaced though, just add "macarm64" to the architectures list on line 42 of discovery.py. Will write a pull request.

john-hen commented 1 year ago

Hi Peter (@pnorgaard ), please note that this is such an easy code change because I made it so. The problem here is not discovering the installation. It's testing, documenting, and thinking this through.

This will be a case where we have two different architectures on the same platform. (Kind of like when there were 32-bit and 64-bit versions of Comsol on Windows/Linux. Though Comsol dropped support for 32-bit before MPh was even created.) As I understand it (and Comsol's documentation on this is minimal), people could have both, the Intel-version and the ARM-based version of Comsol, installed at the same time. There is the Rosetta emulation layer that makes this possible. They could also be running Python through Rosetta, I believe. Anyway, lots of things can go wrong there, and I'm in no position to support users on ARM-based Macs at all.

pnorgaard commented 1 year ago

Understood - yes, I figured the iteration design was intention for such situations, and it did indeed make this quite easy. I was pleasantly surprised that the library behaved quite well with only modification to discovery.py.

If you have recommendation for what kinds of testing provides good coverage, then I'm glad to test on an ARM-based Mac with both the ARM and Intel + Rosetta emulation versions. I might skip on the added fun of python via Rosetta emulation, which feels pretty edge case, at least to me.

I'll revert the pull request, since this chat should at least give users a sharp edged work around.

john-hen commented 1 year ago

Sounds great! 😄

I have some suggestions on how to move forward, but would get back to you with more details some other time. And on that note: Take your time whenever you're busy. We all are.

For the record, I haven't seen a pull request. So I don't think you've created one. (Though I did see you created a fork, with a new branch, and the commit you added there.) Rest assured, I'll merge it eventually once we've figured this out. The more contributors, the merrier.

Most importantly, I need "eyes" on the ARM-based Macs. One thing, for example, that the Comsol documentation does not answer: When you run the installer on your machine, does it give you the option to install: the ARM-based Comsol version, or the Intel-based Comsol version, or both? For Comsol 6.1, that is. Any idea what this was like for Comsol 6.0? They may have changed the installer's default behavior, because ARM was not officially supported until 6.1.

I agree that running Python via Rosetta is an afterthought. We should assume people run Python natively - using the "universal installer", if that's the correct term. But we may have to make that explicit in the "Installation" chapter of the documentation for macOS users.

You should run the test suite on your machine. I "hope" that the various ReadMe files, such as the ones in the tests and tools folder, are helpful enough. If not, I'll explain, and eventually amend them.

fengxiaot commented 1 year ago

I'm running the mph package on Apple M2. pnorgaard's suggestion also worked for me. My COMSOL version is 6.1. Just add "macarm64" to the architectures list. No need to replace "maci64".

pnorgaard commented 1 year ago

"When you run the installer on your machine, does it give you the option to install: the ARM-based Comsol version, or the Intel-based Comsol version, or both?"

Version 6.0 has separate disk images for macOS Intel and macOS M1. Version 6.1 revises the latter to "macOS Apple silicon (M1 and later)". Thus far I've only been using MPh with COMSOL 6.1 on an M1, but I'll try to get around to testing other variations.

Often at mph.start() I encounter the following warning: [0.005s][warning][os,thread] Attempt to protect stack guard pages failed. [0.005s][warning][os,thread] Attempt to deallocate stack guard pages failed.

Which I believe is related to the java VM. I played around with the java preferences in the comsol.ini to see if that would address it, but no luck so far. That said, I haven't actually encountered any problems using MPh.

On Tue, Jul 25, 2023 at 10:15 AM fengxiaot @.***> wrote:

I'm running the mph package on Apple M2. doub1emint's suggestion also worked for me. My COMSOL version is 6.1. Just add "macarm64" to the architectures list. No need to replace "maci64".

— Reply to this email directly, view it on GitHub https://github.com/MPh-py/MPh/issues/80#issuecomment-1650227971, or unsubscribe https://github.com/notifications/unsubscribe-auth/AP2NR2IIW4I7BVFDCXNUQXDXR75M3ANCNFSM5XZ6E33Q . You are receiving this because you were mentioned.Message ID: @.***>

john-hen commented 1 year ago

@pnorgaard Okay, so that means, if I understand that correctly, that you can have both Comsol versions (the ARM-based one and the Intel one) installed in parallel on an ARM-based Mac.

The primary concern here is backward compatibility. So if a new version of MPh is released with support for ARM, it shouldn't break existing installations using the Intel-based Comsol, on either Intel-based or ARM-based devices. And it's only the ARM-based Macs that are a concern. (Intel Macs have been tested.)

Plus, with Comsol 6.0 (but not 6.1), the Intel version was actually recommended, as the ARM version still lacked features, I believe. But we can leave it to the user to make that decision and install the one they want. That is, they'd have to uninstall the Intel-Comsol if they want to use the ARM-based one. (Or rename the folder or something, to hide it from the discovery mechanism.) The documentation then must make it clear that the Intel variant (for a given Comsol version) will be selected (if properly installed), and that the ARM variant will be silently ignored.

In the code, it means we search [maci64, macarm64], in that order, as you suggest. We will assume (as we do now) that Comsol is installed in its default location. Complications that may arise from putting it elsewhere, but possibly having comsol on the search path, are left for the user to deal with.

The documentation should also point out that the support for ARM is "experimental". That's based on the warnings that you report. I have no idea what they mean and where they are issued. Not by MPh, for sure. But this does not affect backward compatibility, so it's fine.

You should still run the entire test suite, not just mph.start(). So that we have a clear picture of what's going on, based on maximum code coverage. Ideally, for Comsol 6.0 as well. And for both Comsol architectures, of course. The test suite should pass without any warnings (I hope) with the Intel-based Comsol. And we want to see if more problems pop up with the ARM variant(s). Hopefully not. 🤞