Fix uri on pc

[javerbukh]

Description

This pull request is to address a problem when loading data from a windows computer. The os.path.sep is different on a windows and linux computer, which does not work for our example notebooks.

Fixes #

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)?

[pllim]

Why was this not caught in CI? Is this not tested on Windows?

[bmorris3]

I might see what went wrong now, thanks @javerbukh. @pllim – it looks like urllib's parse method is for URLs and always uses /. This is a block in the code where the parsed_uri.path is parsed as a URL, which shouldn't be OS specific. Does that make sense?

[pllim]

So, sounds like a new test needs to be added so this case is covered. And this new test should fail on main but pass with this patch. Correct?

[javerbukh]

Do we have a test already of loading data with a windows type path @pllim ? If not, do you have an example of setting up a test like that?

[pllim]

Given that most of the devs work off OSX, probably not. Here's an idea:

1. Find a small test that already use a data file (preferable not remote).
2. When you detect the test is running on Windows, force forward slash, maybe like this: str(p.absolute()).replace("/", "\\") (pathlib has as_posix() but it does not use forward slash when I use git bash shell)
3. See if this fails on main. You want it to fail on main.
4. See if your patch fixes it.

[javerbukh]

I tried that and it does now work, feel free to test on your own windows machines with pytest -s jdaviz/tests/test_utils.py --remote-data .

[javerbukh]

Jira ticket for adding remote data CI tests on Windows OS https://jira.stsci.edu/browse/JDAT-4596

[pllim]

Should we just slap remote data on Windows job as part of this PR and take it back out when that other ticket is done?

[pllim]

Would look something like this: #2924

[javerbukh]

Added that to the CI and it looks like it is fixed in this PR. Thank you for the example!

[bmorris3]

@javerbukh Just checking that this line should remain the same, since in this case the parsed_uri.path is actually a file path?

jdaviz/jdaviz/utils.py

Line 480 in bc5dcbc

local_path = os.path.join(local_path, parsed_uri.path.split(os.path.sep)[-1])

[javerbukh]

@bmorris3 I think you are correct that that should stay the same.

[javerbukh]

@pllim You were correct that the CI should be throwing errors. I ran pytest -s jdaviz/tests/test_utils.py --remote-data on my windows machine and it did in fact error on main. With these changes, that should be fixed. I can create a follow up ticket for finding a way to check remote data tests are run on multiple operating systems.

@bmorris3 , sorry, my first response to you was incorrect. That line does need to be changed.

[pllim]

We could easily enable remote data on Windows job. But that does mean 2x server hit per run.

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.79%. Comparing base (69fabb7) to head (64e161d).
Report is 147 commits behind head on main.

Files with missing lines	Patch %	Lines
jdaviz/utils.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2923   +/-   ##
=======================================
  Coverage   88.79%   88.79%           
=======================================
  Files         111      111           
  Lines       17217    17217           
=======================================
  Hits        15288    15288           
  Misses       1929     1929           

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

[kecnry]

I'm not too worried about regression coverage since this would be very hard to sneak back in accidentally. But if we think there is more windows-specific remote data handling that should get coverage, then that may be worth it? 🤷‍♂️

Looks like URI support (#2875) was milestoned to 3.11, so maybe this should do the same and just be appended to that same changelog entry.

[pllim]

Windows CI is green even with remote data for this patch, so LGTM. Thanks!