DEV: Fix editable install on Windows #1218

pllim · 2022-03-30T16:44:50Z

Description

This pull request is to improve quality of life for developers that have to develop Jdaviz on Windows.

@maartenbreddels , does this patch look reasonable to you from the voila side?

Fixes #925

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. If @duytnguyendtn okays this, good enough.
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 change log needed? If yes, is it added to CHANGES.rst?
Is a milestone set?
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

codecov · 2022-03-30T16:54:09Z

Codecov Report

Merging #1218 (e0f409b) into main (522392e) will not change coverage.
The diff coverage is n/a.

❗ Current head e0f409b differs from pull request most recent head 3077eb1. Consider uploading reports for the commit 3077eb1 to get more accurate results

@@           Coverage Diff           @@
##             main    #1218   +/-   ##
=======================================
  Coverage   77.99%   77.99%           
=======================================
  Files          90       90           
  Lines        7179     7179           
=======================================
  Hits         5599     5599           
  Misses       1580     1580

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e858b1c...3077eb1. Read the comment docs.

maartenbreddels · 2022-03-30T19:13:38Z

@maartenbreddels , does this patch look reasonable to you from the voila side?

Yes, this looks good.

duytnguyendtn

In the middle of the review, but I'm happy I was able to install without admin rights! I am going to advocate for one thing though: I think we should move away from os.path and move towards pathlib.Path. IMO I think it's a lot easier to work with and clearer to read. I've added a few examples of how I think it could work, but haven't tested it

setup.py

duytnguyendtn · 2022-03-30T19:27:16Z

setup.py

+            if not is_win:
+                rel_source = os.path.relpath(os.path.abspath(
+                    source), os.path.abspath(target_subdir))
+            else:  # relpath does not work if source/target on different Windows disks
+                rel_source = os.path.abspath(source)


I don't fully grok what's happening here, but I have a suspicion that this isn't necessary if you use Paths. You can always switch between absolute and relative paths with Path.absolute() and Path.relative_to(yourPath)

relative_to does not work if one is relative and the other one absolute, so you still need abs path for both to be safe.

duytnguyendtn · 2022-03-30T19:29:21Z

setup.py

+            if not is_win:
+                os.symlink(rel_source, target)
+            else:  # Cannot symlink without relpath or Windows admin priv in some OS versions
+                shutil.copytree(rel_source, target)


Why not try the symlink in case the user DOES have admin rights?

Suggested change

if not is_win:

os.symlink(rel_source, target)

else: # Cannot symlink without relpath or Windows admin priv in some OS versions

shutil.copytree(rel_source, target)

try:

rel_source.symlink_to(target)

except: # Cannot symlink without relpath or Windows admin priv in some OS versions

shutil.copytree(rel_source, target)

Also I always forget the direction of the symlink. Did I get the direction right here?

try-except is more trouble than it's worth because I am trying to fix two different problems on Windows with the same logic:

Both our problems: Admin priviledge

My problem: My source checkout is on D but my Miniconda is on C

Also I think if you try to symlink on Windows, you will need admin priv, no?

Co-authored-by: Duy Tuong Nguyen <[email protected]>

Undo overzealous Path replacements.

pllim · 2022-03-31T02:11:01Z

@duytnguyendtn , I tried to use Path as much as I can. Seems to work for me both natively on Windows (no symlink) and on WSL2 Debian (symlink), but you should double check.

If you still feel uncomfortable, please remove the "trivial" label and get another dev (preferable on Mac OSX) to vet this. They should see the template stuff symlinked to the desired destination. For me, it went to a place under .../Miniconda3/envs/<envname>/share/....

maartenbreddels · 2022-03-31T06:37:14Z

tagging @vidartf in case he's interested in having a windows solution to a dev install for voila templates.

duytnguyendtn

Thanks for the changes! Install worked like a charm on Windows! Unfortunately, I ran into the following install error on macOS (Python 3.10.4). Any thoughts?

Installing collected packages: jdaviz
  Running setup.py develop for jdaviz
    error: subprocess-exited-with-error
    
    × python setup.py develop did not run successfully.
    │ exit code: 1
    ╰─> [7 lines of output]
        running develop
        /private/var/folders/93/k31j59sj2493rzfqxc8r5z0r000171/T/pip-build-env-6pu_hgaf/overlay/lib/python3.10/site-packages/setuptools/command/easy_install.py:144: EasyInstallDeprecationWarning: easy_install command is deprecated. Use build and pip and other standards-based tools.
          warnings.warn(
        /private/var/folders/93/k31j59sj2493rzfqxc8r5z0r000171/T/pip-build-env-6pu_hgaf/overlay/lib/python3.10/site-packages/setuptools/command/install.py:34: SetuptoolsDeprecationWarning: setup.py install is deprecated. Use build and pip and other standards-based tools.
          warnings.warn(
        error: [Errno 2] No such file or directory: '/Users/dnguyen/duydir/gitrepos/jdaviz/envpr1218/share/jupyter/nbconvert/templates/jdaviz-default'
        /Users/dnguyen/duydir/gitrepos/jdaviz/envpr1218/src/jdaviz/share/jupyter/nbconvert/templates/jdaviz-default -> /Users/dnguyen/duydir/gitrepos/jdaviz/envpr1218/share/jupyter/nbconvert/templates/jdaviz-default
        [end of output]
    
    note: This error originates from a subprocess, and is likely not a problem with pip.

pllim · 2022-04-05T16:14:26Z

I have no idea where OSX stores the data files. Does this mean anything to you? Is the path wrong?

No such file or directory: '/Users/dnguyen/duydir/gitrepos/jdaviz/envpr1218/share/jupyter/nbconvert/templates/jdaviz-default'

pllim · 2022-04-05T16:14:47Z

What is the value when you install via the setup command in main?

duytnguyendtn · 2022-04-05T16:20:43Z

I see a copy of the default jdaviz template in srcs:

(envpr1218) (base) dxun:jdaviz dnguyen$ find envpr1218/ -name "jdaviz-default"
envpr1218//src/jdaviz/share/jupyter/voila/templates/jdaviz-default
envpr1218//src/jdaviz/share/jupyter/nbconvert/templates/jdaviz-default

Isn't the symlink supposed to be linking this?

duytnguyendtn · 2022-04-05T16:22:55Z

When I install from main, it looks like the symlink to share exists properly:

(envpr1218) (base) dxun:jdaviz dnguyen$ find envpr1218/ -name "jdaviz-default"
envpr1218//share/jupyter/voila/templates/jdaviz-default
envpr1218//share/jupyter/nbconvert/templates/jdaviz-default
envpr1218//src/jdaviz/share/jupyter/voila/templates/jdaviz-default
envpr1218//src/jdaviz/share/jupyter/nbconvert/templates/jdaviz-default

pllim · 2022-04-05T16:27:16Z

Since you can reproduce the error, are you able to run the code in DevelopCmd.run() one by one and see exactly what went wrong in this patch?

pllim · 2022-04-05T16:27:39Z

Also, why didn't it trigger failure in the OSX CI job?

duytnguyendtn · 2022-04-05T16:28:54Z

Before we go that far, we might want to request someone else to reproduce? (Also considering the OSX job didn't fail?)

javerbukh · 2022-04-05T17:00:21Z

Installs fine for me with OSX and python 3.8.13.

duytnguyendtn · 2022-04-05T17:01:25Z

Sounds like it's just me! Thanks for the check @javerbukh !

duytnguyendtn

Here's my approval for the Windows side! Thanks for fixing this thorn that has been in my side for a while!

pllim · 2022-04-05T17:06:23Z

A'ight.... Let's merge.... 🤞

Thanks for the reviews!

javerbukh · 2022-04-05T23:03:02Z

@pllim I actually ran into the same traceback when testing another PR, @duytnguyendtn did you find a fix for it?

pllim · 2022-04-06T00:30:49Z

Oh no 😱

Should we revert?

javerbukh · 2022-04-06T12:19:37Z

If @duytnguyendtn hasn't found a fix then maybe. Do you know what the cause of that error was?

duytnguyendtn · 2022-04-06T12:35:43Z

Nope, I never figured out what the problem was. I thought it was a transient because I was able to install a different branch that was rebased on top of this one, and with the unit tests passing and @javerbukh clear, I thought it was just something local to that instance... apparently not

duytnguyendtn · 2022-04-06T13:07:44Z

A question that popped into my head, @javerbukh were there any differences between the test you ran #1218 (comment) and when you ran into the issue in #1218 (comment)? I'm also curious to know if you installed in editable mode both times?

javerbukh · 2022-04-06T13:08:04Z

Reverting did fix it for me so I would vote for that. I must have not tested this in a new conda environment because as soon as I created a new one after this was merged I saw the error. I did install in editable mode both times.

rosteen · 2022-04-06T13:17:08Z

I confirmed that I can't install in a fresh environment on Mac/Python 3.9 due to this error, I agree with reverting.

rosteen · 2022-04-06T13:18:13Z

Or opening a bug fix PR if anyone can find the solution quickly.

pllim · 2022-04-06T13:38:24Z

I don't have a Mac to test on, so any patch I try would just be wild guessing. If we revert, I can repatch Windows with the my barebone attempt before Duy wanted the Path stuff.

vidartf · 2022-04-11T14:13:24Z

tagging @vidartf in case he's interested in having a windows solution to a dev install for voila templates.

Thanks. For non-ancient versions of Windows, symlinks work as non-admin as long as you put your computer in "developer mode" (found in the settings panel "Developer settings"). As such, my only request would be to try to use symlinks also on Windows, and only try the copying if for some reason symlinks fail. That makes things much easier for me (I don't get the workaround solution when not needed, and the code doesn't just assume I cannot use symlinks just because I'm on Windows).

maartenbreddels · 2022-04-12T11:27:12Z

True, I think a solution that tries but has a fallback mechanism is probably a better idea.

pllim · 2022-04-29T22:27:47Z

@vidartf and @maartenbreddels , sorry I overlooked your feedback here. I have opened #1282 for follow-up but not sure when I can get back to it.

DEV: Fix editable install on Windows

fe860a4

pllim added trivial Only needs one approval instead of two no-changelog-entry-needed changelog bot directive labels Mar 30, 2022

pllim added this to the 2.5 milestone Mar 30, 2022

github-actions bot added the documentation Explanation of code and concepts label Mar 30, 2022

pllim marked this pull request as ready for review March 30, 2022 17:04

pllim requested review from duytnguyendtn, rosteen, javerbukh, ojustino and kecnry as code owners March 30, 2022 17:04

duytnguyendtn suggested changes Mar 30, 2022

View reviewed changes

pllim and others added 2 commits March 30, 2022 21:33

Use Path in setup develop

a6326e6

Co-authored-by: Duy Tuong Nguyen <[email protected]>

Fix develop for Unix.

3077eb1

Undo overzealous Path replacements.

pllim force-pushed the win-setup-dev branch from e0f409b to 3077eb1 Compare March 31, 2022 02:04

duytnguyendtn reviewed Apr 5, 2022

View reviewed changes

duytnguyendtn approved these changes Apr 5, 2022

View reviewed changes

pllim merged commit 1b55d55 into spacetelescope:main Apr 5, 2022

pllim deleted the win-setup-dev branch April 5, 2022 17:06

pllim mentioned this pull request Apr 29, 2022

DEV: Use symlink on Windows when possible #1282

Closed

pllim mentioned this pull request Jun 2, 2022

DEV: Allow symlink in Windows for editable install #1373

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEV: Fix editable install on Windows #1218

DEV: Fix editable install on Windows #1218

pllim commented Mar 30, 2022 •

edited

Loading

codecov bot commented Mar 30, 2022 •

edited

Loading

maartenbreddels commented Mar 30, 2022

duytnguyendtn left a comment

duytnguyendtn Mar 30, 2022

pllim Mar 31, 2022

duytnguyendtn Mar 30, 2022

pllim Mar 31, 2022

pllim Mar 31, 2022

pllim commented Mar 31, 2022

maartenbreddels commented Mar 31, 2022

duytnguyendtn left a comment

pllim commented Apr 5, 2022

pllim commented Apr 5, 2022

duytnguyendtn commented Apr 5, 2022 •

edited

Loading

duytnguyendtn commented Apr 5, 2022

pllim commented Apr 5, 2022

pllim commented Apr 5, 2022

duytnguyendtn commented Apr 5, 2022

javerbukh commented Apr 5, 2022

duytnguyendtn commented Apr 5, 2022

duytnguyendtn left a comment

pllim commented Apr 5, 2022

javerbukh commented Apr 5, 2022

pllim commented Apr 6, 2022

javerbukh commented Apr 6, 2022

duytnguyendtn commented Apr 6, 2022

duytnguyendtn commented Apr 6, 2022

javerbukh commented Apr 6, 2022

rosteen commented Apr 6, 2022

rosteen commented Apr 6, 2022

pllim commented Apr 6, 2022

vidartf commented Apr 11, 2022

maartenbreddels commented Apr 12, 2022

pllim commented Apr 29, 2022

DEV: Fix editable install on Windows #1218

DEV: Fix editable install on Windows #1218

Conversation

pllim commented Mar 30, 2022 • edited Loading

Description

Checklist for package maintainer(s)

codecov bot commented Mar 30, 2022 • edited Loading

Codecov Report

maartenbreddels commented Mar 30, 2022

duytnguyendtn left a comment

Choose a reason for hiding this comment

duytnguyendtn Mar 30, 2022

Choose a reason for hiding this comment

pllim Mar 31, 2022

Choose a reason for hiding this comment

duytnguyendtn Mar 30, 2022

Choose a reason for hiding this comment

pllim Mar 31, 2022

Choose a reason for hiding this comment

pllim Mar 31, 2022

Choose a reason for hiding this comment

pllim commented Mar 31, 2022

maartenbreddels commented Mar 31, 2022

duytnguyendtn left a comment

Choose a reason for hiding this comment

pllim commented Apr 5, 2022

pllim commented Apr 5, 2022

duytnguyendtn commented Apr 5, 2022 • edited Loading

duytnguyendtn commented Apr 5, 2022

pllim commented Apr 5, 2022

pllim commented Apr 5, 2022

duytnguyendtn commented Apr 5, 2022

javerbukh commented Apr 5, 2022

duytnguyendtn commented Apr 5, 2022

duytnguyendtn left a comment

Choose a reason for hiding this comment

pllim commented Apr 5, 2022

javerbukh commented Apr 5, 2022

pllim commented Apr 6, 2022

javerbukh commented Apr 6, 2022

duytnguyendtn commented Apr 6, 2022

duytnguyendtn commented Apr 6, 2022

javerbukh commented Apr 6, 2022

rosteen commented Apr 6, 2022

rosteen commented Apr 6, 2022

pllim commented Apr 6, 2022

vidartf commented Apr 11, 2022

maartenbreddels commented Apr 12, 2022

pllim commented Apr 29, 2022

pllim commented Mar 30, 2022 •

edited

Loading

codecov bot commented Mar 30, 2022 •

edited

Loading

duytnguyendtn commented Apr 5, 2022 •

edited

Loading