Skip to content

[electrophysiology_browser] On-demand file download tarring - #10481

Merged
driusan merged 3 commits into
aces:29.0-releasefrom
MaximeBICMTL:on-demand-tar-ephys-download
Jun 29, 2026
Merged

[electrophysiology_browser] On-demand file download tarring#10481
driusan merged 3 commits into
aces:29.0-releasefrom
MaximeBICMTL:on-demand-tar-ephys-download

Conversation

@MaximeBICMTL

@MaximeBICMTL MaximeBICMTL commented May 5, 2026

Copy link
Copy Markdown
Contributor

Alternative to #10411

Description

This PR makes two changes;

  • Replace the download links in the electrophysiology browser to use the LORIS API instead of get_file.php.
  • Add a new ArchiveStream class to LORIS core to tar on-demand a file or directory and make it available to download. This new class is adopted in the electrophysiology browser to make it possible to download CTF directories for MEG.

@github-actions github-actions Bot added Language: PHP PR or issue that update PHP code Language: Javascript PR or issue that update Javascript code Module: api PR or issue related to api module Module: electrophysiology_browser PR or issue related to electrophysiology_browser module labels May 5, 2026
@MaximeBICMTL
MaximeBICMTL force-pushed the on-demand-tar-ephys-download branch from 2fcd4ef to fffb14c Compare May 5, 2026 11:37
@MaximeBICMTL MaximeBICMTL added the Category: Feature PR or issue that aims to introduce a new feature label May 5, 2026
@github-project-automation github-project-automation Bot moved this to Todo in LORIS MEG May 5, 2026
@MaximeBICMTL MaximeBICMTL added Category: Security PR or issue that aims to improve security Difficulty: Medium PR or issue that require a moderate effort or expertise to implement, review, or test labels May 5, 2026
@MaximeBICMTL
MaximeBICMTL force-pushed the on-demand-tar-ephys-download branch 4 times, most recently from a1274cd to 933c15c Compare May 13, 2026 03:53
@MaximeBICMTL
MaximeBICMTL marked this pull request as ready for review May 22, 2026 06:34
@MaximeBICMTL
MaximeBICMTL removed the request for review from jeffersoncasimir May 26, 2026 13:00
@MaximeBICMTL

MaximeBICMTL commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Since this approach request came from @driusan I guess I am gonna assign this to you. I tested it locally (of course) but feel free to re-assign it if you prefer.

@MaximeBICMTL
MaximeBICMTL changed the base branch from main to 29.0-release June 15, 2026 13:22
@driusan driusan assigned jeffersoncasimir and unassigned driusan Jun 15, 2026

@jeffersoncasimir jeffersoncasimir left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Most of these endpoints don't work, like /bidsfiles/channels, for example.

The issue was not introduced by this PR but it makes this switch problematic.

This join is responsible for the channels, picked as an example, and channels works for me If I change that line to:

LEFT JOIN physiological_channel pc
ON (pc.PhysiologicalFileID = f.PhysiologicalFileID)

and add this at the end, to similarly obtain one:
GROUP BY pc.PhysiologicalFileID

I didn't look too deeply into why this is the case, but that query can use some love.

That query in the DB returns NULL for everything except filelocation and archive_file_path, prior to my change, which returns the channel_file_path

lmk if you can't reproduce it

@MaximeBICMTL
MaximeBICMTL force-pushed the on-demand-tar-ephys-download branch from 933c15c to 33b6346 Compare June 19, 2026 12:24
@MaximeBICMTL

Copy link
Copy Markdown
Contributor Author

Oh, good catch @jeffersoncasimir, I did not run into that issue during my original tests, but after looking at your explanation and doing more tests, I can reproduce.

I think the problem is that the join subqueries use LIMIT 1 before the join, so if the first entry of each table matches the acquisition file you are in luck (which I guess was the case in my original tests), and in all other cases the query is broken. I changed the query to use subqueries rather than a join, and use a where condition to get the correct entries.

I rebased on 29-release and added a commit to fix the query, it fixed it for me, tell me if you are still having issues.

@jeffersoncasimir jeffersoncasimir left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Works great!

@driusan
driusan merged commit 61d2d27 into aces:29.0-release Jun 29, 2026
21 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in LORIS MEG Jun 29, 2026
kongtiaowang pushed a commit to kongtiaowang/Loris that referenced this pull request Jul 2, 2026
This makes two changes;
- Replace the download links in the electrophysiology browser to use the
LORIS API instead of `get_file.php`.
- Add a new `ArchiveStream` class to LORIS core to tar on-demand a file
or directory and make it available to download. This new class is
adopted in the electrophysiology browser to make it possible to download
CTF directories for MEG.
HachemJ pushed a commit to HachemJ/Loris that referenced this pull request Jul 20, 2026
This makes two changes;
- Replace the download links in the electrophysiology browser to use the
LORIS API instead of `get_file.php`.
- Add a new `ArchiveStream` class to LORIS core to tar on-demand a file
or directory and make it available to download. This new class is
adopted in the electrophysiology browser to make it possible to download
CTF directories for MEG.
HachemJ pushed a commit to HachemJ/Loris that referenced this pull request Jul 22, 2026
This makes two changes;
- Replace the download links in the electrophysiology browser to use the
LORIS API instead of `get_file.php`.
- Add a new `ArchiveStream` class to LORIS core to tar on-demand a file
or directory and make it available to download. This new class is
adopted in the electrophysiology browser to make it possible to download
CTF directories for MEG.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Category: Feature PR or issue that aims to introduce a new feature Category: Security PR or issue that aims to improve security Difficulty: Medium PR or issue that require a moderate effort or expertise to implement, review, or test Language: Javascript PR or issue that update Javascript code Language: PHP PR or issue that update PHP code Module: api PR or issue related to api module Module: electrophysiology_browser PR or issue related to electrophysiology_browser module

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants