Skip to content

JP2Grok: support native Grok decompression of vsicurl files#14333

Open
boxerab wants to merge 2 commits into
OSGeo:masterfrom
boxerab:grok_vsicurl
Open

JP2Grok: support native Grok decompression of vsicurl files#14333
boxerab wants to merge 2 commits into
OSGeo:masterfrom
boxerab:grok_vsicurl

Conversation

@boxerab

@boxerab boxerab commented Apr 11, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

@rouault will enjoy this minimal PR : allow JP2Grok driver to use native Grok decompression for vsicurl files

AI tool usage

  • AI supported my development of this PR.

Tasklist

Comment thread frmts/grok/grkdatasetbase.h Outdated
@boxerab boxerab force-pushed the grok_vsicurl branch 3 times, most recently from 5cc6aa9 to d40ef10 Compare April 12, 2026 09:37

@rouault rouault left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be properly rebased on top of latest master to solve merge conflict

I do believe that the network integration is fragile, and could cause headaches to users that use a GDAL networking option (or S3 authentication scheme) that isn't translated into Grok. I would certainly have strong reservations if Grok was a default driver.

Comment thread frmts/grok/grkdatasetbase.h
@boxerab boxerab force-pushed the grok_vsicurl branch 3 times, most recently from d0b4dac to 5392425 Compare April 12, 2026 16:37
@rouault

rouault commented Apr 12, 2026

Copy link
Copy Markdown
Member

Aaron, I'm giving up. I'll stop reviewing your PRs. I just can't psychology deal with being at the receiving end of LLM generated PRs. I don't know how to explain it, but it makes me mentally very uncomfortable, and it is exhausting. If another maintainer feels up for the task, fine to them.

@rouault rouault added the AI assisted⚠️ AI assisted coding involved. Review with extreme scepticism. label Apr 12, 2026
@aaron-boxer

Copy link
Copy Markdown

Aaron, I'm giving up. I'll stop reviewing your PRs. I just can't psychology deal with being at the receiving end of LLM generated PRs. I don't know how to explain it, but it makes me mentally very uncomfortable, and it is exhausting. If another maintainer feels up for the task, fine to them.

No problem, I understand. I will not be making any more AI assisted PRs for this driver.
How about we wait a few weeks and then merge this one as it is already written with passing tests ?

@boxerab boxerab force-pushed the grok_vsicurl branch 2 times, most recently from 0d1f19b to 7c0cfe0 Compare May 2, 2026 20:08
@boxerab

boxerab commented May 2, 2026

Copy link
Copy Markdown
Contributor Author

Hello again @rouault , hope all is well. Three weeks have passed since opening this relatively small PR - half of the changes are unit tests, all passing. The feature is a useful one IMHO as it allows Grok to efficiently handle more network files, and a conservative approach is taken for different auth options - those unsupported by Grok will trigger GDAL VSILFILE handling via callback.

Please consider accepting this one. 🙏

@rouault

rouault commented May 7, 2026

Copy link
Copy Markdown
Member

I'm not convinced that letting Grok handle I/O is the way to go. It will ultimately lead to issues similar to #14508 where there's a discrepancy between the library re-implementation of the GDAL protocol.
Maybe the thing to explore would be to identify what would be missing in GDAL VSIVirtualHandle , if anything is missing, and improve the brige with Grok I/O layer

@boxerab

boxerab commented May 8, 2026

Copy link
Copy Markdown
Contributor Author

I'm not convinced that letting Grok handle I/O is the way to go. It will ultimately lead to issues similar to #14508 where there's a discrepancy between the library re-implementation of the GDAL protocol. Maybe the thing to explore would be to identify what would be missing in GDAL VSIVirtualHandle , if anything is missing, and improve the brige with Grok I/O layer

agree, we don't want to have more issues like #14508.

The way IO currently works is that GDAL manages authentication, and then passes the url and any auth information like custom headers to Grok, which then reads the file. In the case of network files, it uses libcurl directly in a way that is customized for J2K file format, making use of the main header and TLM markers if present to very efficiently pull the data, using range requests for tiles. It's not re-implementing the GDAL protocol, which is more general.

Replicating the Grok https fetcher in GDAL would be a lot of work and would only be suitable for one format : J2K. Not sure this is worth it, IMHO.

@rouault

rouault commented May 8, 2026

Copy link
Copy Markdown
Member

if present to very efficiently pull the data, using range requests for tiles

That's vague. Concretely, what type of calls is missing in VSIVirtualHandle to implement efficient reading from grok ?

@boxerab boxerab force-pushed the grok_vsicurl branch 2 times, most recently from fc26887 to 346582f Compare May 10, 2026 11:36
@boxerab

boxerab commented May 10, 2026

Copy link
Copy Markdown
Contributor Author

if present to very efficiently pull the data, using range requests for tiles

That's vague. Concretely, what type of calls is missing in VSIVirtualHandle to implement efficient reading from grok ?

Grok makes lots of async liburl fetches based on JP2 main and tile headers. So for efficiency, Grok has to drive the io and it has to be asynchronous. VSIVirtualHandle is synchronous, so an async version would need to be built.

@boxerab boxerab force-pushed the grok_vsicurl branch 2 times, most recently from b9e3a78 to 49765c8 Compare May 29, 2026 10:26
@boxerab boxerab force-pushed the grok_vsicurl branch 2 times, most recently from 0102ec4 to a93c41b Compare June 6, 2026 21:41
User must set OPJLIKE_VSICURL_NATIVE=YES to opt in, otherwise
GDAL virtual file system is used to read from network.
@aaron-boxer

Copy link
Copy Markdown

@rouault I have added another commit to make native Grok vsicurl support opt in - unless user sets OPJLIKE_VSICURL_NATIVE=YES, the old behavior without this PR is what happens. So users have the option, if they choose.

@rouault

rouault commented Jun 10, 2026

Copy link
Copy Markdown
Member

I have added another commit to make native Grok vsicurl support opt in - unless user sets OPJLIKE_VSICURL_NATIVE=YES, the old behavior without this PR is what happens. So users have the option, if they choose.

I believe the option should cover both /vsicurl/ and /vsis3/ (differing by default to Grok was inappropriate IMHO) since there's no reason to have different defaults for both, and it should likely be renamed something like "GROK_NATIVE_NETWORKING" as it has nothing to do with openjpeg. And should be mentionned in jp2grok.rst

@boxerab

boxerab commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

I believe the option should cover both /vsicurl/ and /vsis3/ (differing by default to Grok was inappropriate IMHO) since there's no reason to have different defaults for both, and it should likely be renamed something like "GROK_NATIVE_NETWORKING" as it has nothing to do with openjpeg. And should be mentionned in jp2grok.rst

makes sense, will get this in shortly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI assisted⚠️ AI assisted coding involved. Review with extreme scepticism.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants