Show path to file #2896

gibsongreen · 2024-05-28T20:34:36Z

Description

This pull request is to address displaying the default file path in the Export plugin UI. The default path shall update if the user inputs a relative or absolute path into the filename input field in the UI.

It also re-enables exporting for them the API. If a user provides a path via in the API, the file shall be exported there.

In the UI, the default filepath will be used (cwd), or, if a user provides a path, it will export to that location instead of exporting only to the users Downloads directory.

Follow up tickets are needed to address:

updating file path when the user is in standalone
updating mp4 exporting (on main, exporting mp4 results in a traceback, with this PR the mp4 downloads and is viewable, but the video is white screen with no content)
loading images from the import directory interface in standalone does not work

Change log entry

Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
Do the proposed changes follow the STScI Style Guides?
Are tests added/updated as required? If so, do they follow the STScI Style Guides?
Are docs added/updated as required? If so, do they follow the STScI Style Guides?
Did the CI pass? If not, are the failures related?
Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone. Bugfix milestone also needs an accompanying backport label.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

codecov · 2024-05-28T20:58:04Z

Codecov Report

Attention: Patch coverage is 71.42857% with 8 lines in your changes missing coverage. Please review.

Project coverage is 88.80%. Comparing base (0ffae63) to head (79c8851).
Report is 153 commits behind head on main.

Files with missing lines	Patch %	Lines
jdaviz/configs/default/plugins/export/export.py	71.42%	8 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2896   +/-   ##
=======================================
  Coverage   88.79%   88.80%           
=======================================
  Files         111      111           
  Lines       17223    17234   +11     
=======================================
+ Hits        15293    15304   +11     
  Misses       1930     1930

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

camipacifici · 2024-05-29T13:56:10Z

Would it be possible to have standalone point at the local folder? I remember @pllim has done something to this effect in the past. As it is now, it points to /private/ (see screenshot).
@kecnry, did we decide to wait for Solara to make the path editable? If so, we just need the bug fixes for saving in the appropriate place.

Thank you!

kecnry · 2024-05-29T14:07:44Z

I think the immediate improvement is to just display what is currently being adopted so that it is clear where they are going and then we can improve the behavior (or allow choosing the directory) either as a follow-up or when using solara. EDIT: unless something is super-easy now, of course!

We probably do want this to be smart enough to handle when the user passes an absolute path (or even ~ or .. in the path) - is that reasonable here or out-of-scope?

pllim · 2024-05-29T14:09:24Z

I thought I commented about the standalone workaround but maybe it was a different PR.

jdaviz/jdaviz/configs/default/plugins/export/export.py

Lines 346 to 350 in ec80391

    
           filepath = filename.parent 
        
           if filepath and not filepath.is_dir(): 
        
               raise ValueError(f"Invalid path={filepath}") 
        
           elif ((not filepath or str(filepath).startswith(".")) and os.environ.get("JDAVIZ_START_DIR", "")):  # noqa: E501 # pragma: no cover 
        
               filename = os.environ["JDAVIZ_START_DIR"] / filename

gibsongreen · 2024-05-29T14:21:29Z

I think the immediate improvement is to just display what is currently being adopted so that it is clear where they are going and then we can improve the behavior (or allow choosing the directory) either as a follow-up or when using solara. EDIT: unless something is super-easy now, of course!

We probably do want this to be smart enough to handle when the user passes an absolute path (or even ~ or .. in the path) - is that reasonable here or out-of-scope?

The ticket was made pre-Export plugin with the main objective being to display the location where file(s) are supposed to be stored. There are 2 follow up tickets in the backlog: one for address the bug relating to where the file(s) actually get stored (the UI and API have variable behavior), and a ticket to enable the user to select their desired directory with which the file(s) would be stored (defaulting to the Jupyter cwd).

If this programmatic methodology works for reviewers, I can continue to build these improvements within this PR!

camipacifici · 2024-05-29T14:32:10Z

Showing the path is done, but the behavior is still not ideal. I propose, in order of priority:

make the path the same for standalone and in notebook (using the working directory, like it is now in notebook)
fix bug when actually saving the file.

For later: make the path editable.
I do not have preferences whether the two things happen in separate PRs or in this one.

gibsongreen · 2024-05-30T19:19:52Z

Updated behavior to basically handle relative paths:

Screen.Recording.2024-05-30.at.3.09.37.PM.mov

In this, the filename is a part of the filepath, I can remove it if that is preferred.

bmorris3

I'm confused – even though there's a "file path export location", when I click export, my file goes to ~/Downloads/. I see there's discussion about doing something different in follow-ups, but this makes the appearance of a bug.

camipacifici · 2024-06-18T19:40:15Z

Thank you for the work!
The files don't get saved in downloads anymore, yay!

It works well in the notebook. In standalone, it does save in the current directory, but it still tells me it will save in a temporary directory:

Another thing I noticed is that the warning for the existing file name appears when opening the plugin instead of after clicking Export. If I click Cancel and then Export, it correctly appears again.

There is a problem in standalone loading images from the import button. This is present also in main, but not in 3.10.2. I do not have this problem if I use the import button in the notebook or if I select the file from the launcher before clicking Imviz in standalone.

gibsongreen · 2024-06-18T20:51:23Z

An additional ticket is in this sprint that shall address the issue of the file path location in standalone pointing to the current working directory (or the directory the user selects in the filename section). I do have a mock solution, but have been unable to test it in an editable environment of standalone. I will bring this up to troubleshoot in the next tagup so we can get it tested and in ASAP. (When I get this troubleshooted I will also look into loading images into the viewer too).

Regarding the Overwrite issue, I believe that this is the intended behavior. My assumption is that there was an existing file with the same name imviz-0.png and extension in the file path directory that was selected. Since it exists already, the user should receive a warning in case they may forgotten that had this file already created. Before this PR, that wouldn't happen because the file would be downloaded to the users Downloads, and in this scenario, an imviz-0-1.png would be created automatically by the OS to prevent filename collision.

camipacifici · 2024-06-18T21:26:02Z

I would expect to get the warning when I click Export, not when I first open the plugin, but I guess this is up for debate :)
We can take a poll at the next tag up.

Thank you for reminding me about the other ticket. I hope you manage to troubleshoot the standalone editable environment issue soon.

gibsongreen · 2024-06-18T22:04:48Z

I would expect to get the warning when I click Export, not when I first open the plugin, but I guess this is up for debate :) We can take a poll at the next tag up.

Thank you for reminding me about the other ticket. I hope you manage to troubleshoot the standalone editable environment issue soon.

Sorry, I misread the part about Exporting when the plugin was first opened, I thought it had to do with no receiving the overwrite warning in general. I am all for making sure when the user reopens Export with a file previously downloaded in their directory, to initially show the standard UI and not have the overwrite warning prompted to them until the they chose to export that exact filename/file path

bmorris3

I tried to specify a path like so:

and the file got saved to my Downloads folder with the path: _Users_bmmorris_Desktop_imviz-0.png. Is that expected?

bmorris3 · 2024-06-21T19:24:18Z

jdaviz/configs/default/plugins/export/export.py

+        # Update the UI Filepath if relative or absolute paths are provided
+        # by user via self.filename_value
+        if '~' in self.filename_value and '/' in self.filename_value:
+            expanded_path = os.path.expanduser(self.filename_value)
+            abs_path = Path(expanded_path).resolve()
+            self.default_filepath = str(abs_path)
+        elif '/' in self.filename_value and \
+             ('.' in self.filename_value or '..' in self.filename_value):
+            abs_path = Path(self.filename_value).resolve()
+            self.default_filepath = str(abs_path)
+        elif '/' in self.filename_value:
+            self.default_filepath = str(self.filename_value)


The file path separator / is specific to unix machines. I don't expect this to work as expected on Windows. Is there any reason not to resolve every file path with the same Path methods?

bmorris3 · 2024-06-21T19:28:54Z

jdaviz/configs/default/plugins/export/tests/test_export.py

+        export_plugin.filename_value = '/img.png'
+        assert os.path.abspath(export_plugin.default_filepath) == os.path.abspath(export_plugin.filename_value)  # noqa: E501
+
+        export_plugin.filename_value = '~/img.png'
+        expected_path = os.path.normpath(os.path.expanduser('~/img.png'))
+
+        assert export_plugin.default_filepath == expected_path
+
+        export_plugin.filename_value = '../img.png'
+        expected_path = os.path.abspath('../img.png')
+
+        assert export_plugin.default_filepath == expected_path
+
+        export_plugin.filename_value = './img.png'
+        expected_path = os.path.abspath('./img.png')


You might want to create these filenames with OS independent methods for building paths like Path('.') / 'image.png'.

I'm all for these changes, thank you for comment (and the one above), both will be implemented shortly!

gibsongreen · 2024-06-21T19:45:34Z

I tried to specify a path like so:
and the file got saved to my Downloads folder with the path: `_Users_bmmorris_Desktop_imviz-0.png`. Is that expected?

This is unexpected, I passed the same input and got the expected results (the correct filename, in the expected directory). I have seen the behavior your screenshot displays before, I'm unsure why we're seeing different results from me. @camipacifici whenever you have the time, do you mind testing this and see whether the file name gets interpreted as a string or a Path

camipacifici · 2024-06-25T19:07:56Z

To me it never saves to Downloads, but I found another couple of things.
In jupyterlab or notebook:
If I try to save the plot option histogram without having opened the plot option plugin, it gives me a red snackbar

If I first open the plot option plugin, then save the histogram, everything works. If I then close the plugin and I try to save, again it works.
With the image, if I already have the file on disk, I get the warning to cancel or overwrite. If I try to resave the histogram, it just overwrites (does not give me the warning).
In standalone:
All of the above +
After the warning, when nothing is selected, the path reverts to the /private/ folder. If I click to save the image or the histogram, it goes back to the correct local directory.

Except the last, the others are not strictly related to the path to file. Let me know if you want me to create tickets or you will do it. Thank you!!

gibsongreen · 2024-06-26T13:56:24Z

To me it never saves to Downloads, but I found another couple of things. In jupyterlab or notebook: If I try to save the plot option histogram without having opened the plot option plugin, it gives me a red snackbar If I first open the plot option plugin, then save the histogram, everything works. If I then close the plugin and I try to save, again it works. With the image, if I already have the file on disk, I get the warning to cancel or overwrite. If I try to resave the histogram, it just overwrites (does not give me the warning). In standalone: All of the above + After the warning, when nothing is selected, the path reverts to the /private/ folder. If I click to save the image or the histogram, it goes back to the correct local directory.

Except the last, the others are not strictly related to the path to file. Let me know if you want me to create tickets or you will do it. Thank you!!

@camipacifici Thank you for the extensive testing! I went through and recreated each of the issues that exist. I will start making tickets for each of them now.

Regarding the last issue in standalone where the path reverts to /private/, I will double check today with the devs, but I believe the default is that there is always a selection made in the UI for Export plugin. The moment another selection is made, the standalone file path will then update to the cwd again. Ultimately, I believe this issue comes from the underlying bug(s) with the histogram, and will be resolved if we ensure the histogram exists even if the tray has yet to be opened for Plot Options, (or a less ideal temporary solution is disable the histogram export and disable a message to the user the must open Plot Options tray to be able to export the histogram).

UPDATE: Tickets are now in the backlog

bmorris3

This issue may not be related to this PR, but here's a workflow that confused me. I open data in Imviz, open Export, try to export the plot options histogram. The file path defaults disappear, and no file is saved:

imviz-export-no-hist.mov

If you do the same workflow, but open plot options first before trying to export the histogram, it works fine. So I expect this is just a problem of trying to export a figure that hasn't been rendered. Should we remove export options for plugin figures that aren't yet rendered?

kecnry · 2024-07-02T15:10:46Z

I open data in Imviz, open Export, try to export the plot options histogram. The file path defaults disappear, and no file is saved

See #2934 (the disappearing selection is not yet fixed, but will be there soon).

gibsongreen · 2024-07-02T15:14:50Z

@bmorris3 this is actually what Kyle's PR is addressing. The PR handles the case where plot options hasn't been opened but the user tries to download the histogram. The other bug that is somewhat associated and will likely be in Kyle's PR is when you do export the histogram, the filename is cleared and no selection is made (has to do with the list of choices being built from scratch every time you export, when we actually want the current selection to be maintained): #2934

bmorris3

In light of the fact that my only concerns have pending fixes in other PRs, this looks good 👌🏻

kecnry · 2024-07-03T12:20:05Z

where do we want this milestoned? #2934 is milestoned for the next bugfix release. This should maybe either go there as well or in 3.11 (in case that ends up existing)?

camipacifici · 2024-07-03T16:31:26Z

I found one more thing. Sorry for not catching it before.
If I use the markers plugin and then I go to export to save out the markers table:

saving in ecsv or fits works
saving in csv does not work (i am ok removing the option)
resaving keeping the same name brings up the warning dialog, but hitting "overwrite" does nothing (the dialog does not disappear)

camipacifici · 2024-07-03T16:39:13Z

I approve this for its scope. Please file another bug ticket for the markers behavior.

…port to filepath

…th shows cwd and not temp path

…or building paths

* show default filepath in Export plugin * update filepath if ., .., and/or / provided by user in filename * add ~ relative path, revert to PosixPath instead of string, UI/API export to filepath * add some test coverage * update test, reduce transparency of overwrite overlay as temporary fix * add / testing * address 3.10 test failures * address 3.10 test failures * ensure overwrite only occurs after export button click, standalone path shows cwd and not temp path * update paths to use os/Path and tests to use OS independent methods for building paths * update change log milestone

gibsongreen requested review from rosteen, javerbukh, pllim, kecnry, bmorris3, haticekaratay and cshanahan1 as code owners May 28, 2024 20:34

github-actions bot added the plugin Label for plugins common to multiple configurations label May 28, 2024

gibsongreen added this to the 4.0 milestone May 28, 2024

gibsongreen added the UI/UX😍 label May 28, 2024

gibsongreen force-pushed the show_path_to_file branch from 2f7347d to 97bb2e6 Compare May 30, 2024 19:18

bmorris3 reviewed May 31, 2024

View reviewed changes

gibsongreen marked this pull request as draft June 11, 2024 20:02

gibsongreen force-pushed the show_path_to_file branch from e71970e to 3e8b44c Compare June 11, 2024 21:50

gibsongreen added the bug Something isn't working label Jun 11, 2024

gibsongreen marked this pull request as ready for review June 14, 2024 20:28

bmorris3 reviewed Jun 21, 2024

View reviewed changes

gibsongreen force-pushed the show_path_to_file branch from 905b82e to c3630b9 Compare June 21, 2024 20:26

bmorris3 reviewed Jul 2, 2024

View reviewed changes

bmorris3 approved these changes Jul 2, 2024

View reviewed changes

gibsongreen modified the milestones: 4.0, 3.11, 3.10.3 Jul 3, 2024

gibsongreen added 10 commits July 5, 2024 12:29

show default filepath in Export plugin

23b67cf

update filepath if ., .., and/or / provided by user in filename

ce1146d

add ~ relative path, revert to PosixPath instead of string, UI/API ex…

95031fa

…port to filepath

add some test coverage

4d16d87

update test, reduce transparency of overwrite overlay as temporary fix

9fe1234

add / testing

c245d8d

address 3.10 test failures

e5f2993

address 3.10 test failures

b50dbcf

ensure overwrite only occurs after export button click, standalone pa…

2461e3f

…th shows cwd and not temp path

update paths to use os/Path and tests to use OS independent methods f…

b221862

…or building paths

gibsongreen force-pushed the show_path_to_file branch from c3630b9 to b221862 Compare July 5, 2024 16:29

update change log milestone

79c8851

gibsongreen merged commit af4570e into spacetelescope:main Jul 8, 2024
18 of 19 checks passed

gibsongreen mentioned this pull request Jul 9, 2024

Fix Cubeviz movie export #2942

Merged

kecnry mentioned this pull request Jul 22, 2024

manual backport of PR #3096 #3105

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show path to file #2896

Show path to file #2896

gibsongreen commented May 28, 2024 •

edited

Loading

codecov bot commented May 28, 2024 •

edited

Loading

camipacifici commented May 29, 2024

kecnry commented May 29, 2024 •

edited

Loading

pllim commented May 29, 2024

gibsongreen commented May 29, 2024

camipacifici commented May 29, 2024 •

edited

Loading

gibsongreen commented May 30, 2024 •

edited

Loading

bmorris3 left a comment

camipacifici commented Jun 18, 2024

gibsongreen commented Jun 18, 2024

camipacifici commented Jun 18, 2024

gibsongreen commented Jun 18, 2024

bmorris3 left a comment

bmorris3 Jun 21, 2024

bmorris3 Jun 21, 2024

gibsongreen Jun 21, 2024

gibsongreen commented Jun 21, 2024

camipacifici commented Jun 25, 2024

gibsongreen commented Jun 26, 2024 •

edited

Loading

bmorris3 left a comment

kecnry commented Jul 2, 2024

gibsongreen commented Jul 2, 2024

bmorris3 left a comment

kecnry commented Jul 3, 2024

camipacifici commented Jul 3, 2024

camipacifici commented Jul 3, 2024

Show path to file #2896

Show path to file #2896

Conversation

gibsongreen commented May 28, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented May 28, 2024 • edited Loading

Codecov Report

camipacifici commented May 29, 2024

kecnry commented May 29, 2024 • edited Loading

pllim commented May 29, 2024

gibsongreen commented May 29, 2024

camipacifici commented May 29, 2024 • edited Loading

gibsongreen commented May 30, 2024 • edited Loading

bmorris3 left a comment

Choose a reason for hiding this comment

camipacifici commented Jun 18, 2024

gibsongreen commented Jun 18, 2024

camipacifici commented Jun 18, 2024

gibsongreen commented Jun 18, 2024

bmorris3 left a comment

Choose a reason for hiding this comment

bmorris3 Jun 21, 2024

Choose a reason for hiding this comment

bmorris3 Jun 21, 2024

Choose a reason for hiding this comment

gibsongreen Jun 21, 2024

Choose a reason for hiding this comment

gibsongreen commented Jun 21, 2024

camipacifici commented Jun 25, 2024

gibsongreen commented Jun 26, 2024 • edited Loading

bmorris3 left a comment

Choose a reason for hiding this comment

kecnry commented Jul 2, 2024

gibsongreen commented Jul 2, 2024

bmorris3 left a comment

Choose a reason for hiding this comment

kecnry commented Jul 3, 2024

camipacifici commented Jul 3, 2024

camipacifici commented Jul 3, 2024

gibsongreen commented May 28, 2024 •

edited

Loading

codecov bot commented May 28, 2024 •

edited

Loading

kecnry commented May 29, 2024 •

edited

Loading

camipacifici commented May 29, 2024 •

edited

Loading

gibsongreen commented May 30, 2024 •

edited

Loading

gibsongreen commented Jun 26, 2024 •

edited

Loading