Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for reading from IOBuffers #55

Merged
merged 3 commits into from
Nov 18, 2024

Conversation

joegilkes
Copy link
Contributor

My solution to the problem presented in #46 was workable but not great:

  • Creating frames from Julia strings required using the slower Python implementation of read_frames through PythonCall.
  • Frames created this way needed significant postprocessing to deal with all of the pythonic types of underlying elements so that they could be written with write_frames, as this needed everything to be a Julia type and not a PythonCall wrapper type.

To (hopefully) provide a better solution, I've implemented a new method of cfopen that internally calls the fmemopen C function. This returns what is functionally a 'file pointer' that the rest of ExtXYZ can work with as if it were an actual file.

I've extended the read_frame methods to allow for passing IOBuffers and added a test case for equivalence with the regular read-from-file approach. I haven't added an equivalent method for write_frame yet but may do so after some testing.

The only bit I'm not sure about is the Windows ccall to fmemopen - I see that there's a special case for handling fdopen but I'm not familiar with how this works on Windows so please correct this if it needs to be.

@jameskermode
Copy link
Member

jameskermode commented Nov 15, 2024

Thanks for the contribution, this sounds useful and I'll be happy to accept once it works on all platforms. However, it's not working on Windows, probably for the reasons you mention. Would you be able to take a look via the CI logs? I don't have a Windows machine I can offer for testing on, unfortunately.

@joegilkes
Copy link
Contributor Author

It seems like there isn't an equivalent of fmemopen on Windows, so I can only really see two solutions:

  1. I package a 3rd party library like https://github.com/Arryboom/fmemopen_windows as a JLL so we can call a Windows-based approximation of it. This would then mean ExtXYZ.jl has to depend on this library.
  2. Reading from IOBuffers remains a Linux/MacOS-only feature, reading from an IOBuffer on Windows raises an error and the unit test gets disabled on Windows.

@jameskermode
Copy link
Member

Solution 2 is fine for now, someone else who want this to work on Windows could still do 1 in future.

@joegilkes
Copy link
Contributor Author

I added a note in the read_frame/read_frames docstrings that it's not supported on Windows, hopefully CI should run fine now.

@jameskermode
Copy link
Member

Still failing on Windows with Julia 1.6 I'm afraid. We could consider this to be very old and update the CI to use 1.10 which is the new LTS version of Julia.

@joegilkes
Copy link
Contributor Author

It looks like @test_throws was updated to allow this syntax at some point after 1.6, so this test fails:

@test_throws "not supported on Windows" read_frames(iob)

I can modify the test to be less specific very easily, unless you want to update the CI? I don't mind either way.

@jameskermode
Copy link
Member

CI updated in #56. If you rebase on main then CI should now pass.

@joegilkes
Copy link
Contributor Author

Not sure what's going on with the macOS CI, seems to be a problem with an AtomsBase test but I haven't touched it?

@joegilkes
Copy link
Contributor Author

Now very not sure what's happening... I slightly messed up the initial rebase so I redid it, and now the CI is running fine? For context, here was the CI log that previously failed and now passes: https://github.com/libAtoms/ExtXYZ.jl/actions/runs/11889607646/job/33126517896

I think there may be some inconsistency with the "Conversion AtomsBase -> Atoms" tests since they use randomly generated atom coords, but it doesn't seem linked to this PR.

@jameskermode jameskermode merged commit c62f812 into libAtoms:master Nov 18, 2024
7 checks passed
@jameskermode
Copy link
Member

Looks OK now so I'll go ahead and merge. Would a new release be useful?

@joegilkes
Copy link
Contributor Author

I would like to use it soon because it tidies up a bunch of my code to not have to use my Python hack, so if you have time then that would be appreciated. I can always run from master for the time being though so no rush!

@joegilkes joegilkes deleted the iobuffer_read branch November 18, 2024 21:08
@jameskermode
Copy link
Member

v0.2.1 should show up in the General index shortly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants