Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix version in setup.py #506

Merged

Conversation

melven
Copy link
Contributor

@melven melven commented Mar 15, 2020

Description

The old code adjusted sys.path and imported heat/core/version which
implicitly imports heat itself. This leads to two problems:

  • setup cannot run without dependencies,
    e.g. just "python setup.py --version" fails when py-torch is not available
  • setuptools skips copying the complete heat code into the target
    directory in some cases as heat is already in sys.path, see https://setuptools.readthedocs.io/en/latest/setuptools.html:

    --always-copy, -a
    Copy all needed distributions to the staging area, even if they are already present in another directory on sys.path. By default, if a requirement can be met using a distribution that is already available in a directory on sys.path, it will not be copied to the staging area.

The new variant directly executes the version.py file and avoids the problems above.

Issue/s resolved: #

Changes proposed:

Execute version.py in setup.py directly instead of importing heat.

Type of change

Remove irrelevant options:

  • Bug fix (non-breaking change which fixes an issue)

Due Diligence

  • All split configurations tested
  • Multiple dtypes tested in relevant functions
  • Documentation updated (if needed)
  • Updated changelog.md under the title "Pending Additions"

Does this change modify the behaviour of other functions? If so, which?

no

The old code adjusted sys.path and imported heat/core/version which
implicitly imports heat itself. This leads to two problems:
 * setup tools cannot run without dependencies,
   e.g. just "python setup.py --version" fails
 * setuptools skip copying the complete heat code into the target
   directory in some cases as heat is already in sys.path
@coquelin77
Copy link
Member

this is failing in pre-commit. please see the README for how to set this up

@coquelin77
Copy link
Member

this is still failing. is there any update on this?

@Markus-Goetz
Copy link
Member

Bump

@coquelin77 coquelin77 closed this Apr 7, 2020
@Markus-Goetz Markus-Goetz reopened this Apr 7, 2020
@codecov
Copy link

codecov bot commented Apr 7, 2020

Codecov Report

Merging #506 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #506      +/-   ##
==========================================
+ Coverage   96.35%   96.38%   +0.02%     
==========================================
  Files          75       75              
  Lines       14585    14481     -104     
==========================================
- Hits        14054    13958      -96     
+ Misses        531      523       -8     
Impacted Files Coverage Δ
heat/core/arithmetics.py 99.15% <0.00%> (-0.05%) ⬇️
heat/core/tests/test_arithmetics.py 98.44% <0.00%> (+0.56%) ⬆️
heat/core/operations.py 94.07% <0.00%> (+1.30%) ⬆️

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 d92bff9...d30588b. Read the comment docs.

@coquelin77
Copy link
Member

the line that you removed markus was there to satisfy flake. __version__ will be initialized in the exec but flake doesnt know that so it fails

@melven
Copy link
Contributor Author

melven commented Apr 8, 2020

Hi

Sorry, I didn't answer earlier. I'm not working most of the time, currently.
(I just took holidays to play with the kids at home)

So feel free to fix/improve this (thanks!).

I can also look at it after Easter.

@Markus-Goetz Markus-Goetz self-requested a review April 8, 2020 14:34
@Markus-Goetz Markus-Goetz merged commit 695c06a into helmholtz-analytics:master Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants