changes to setup, remove teal #63

cshanahan1 · 2022-02-17T03:33:29Z

Made changes to setup files to install dependencies and remove 'relic' I also removed some pytest setup since there are currently no tests in this package. I also removed everything related to 'teal'.

pllim

I am not sure if this is enough. I don't see any pyproject.toml . Also there are still metadata left in setup.py that can be moved to setup.cfg.

You also want to try to actually package this thing and make sure you can install both the packaged tarball and wheel (separately) and get back a working install. If you want to take this a step further, you can automate like https://github.com/astropy/pytest-doctestplus/blob/main/.github/workflows/publish.yml .

It is unfortunate that this package does not have a test suite, so all you can do is to make sure the installation works (i.e., you can import the package) but not guarantee that it would work correctly.

pllim · 2022-02-18T20:49:04Z

p.s. I see Matt R put in https://github.com/spacetelescope/wfc3tools/blob/master/.github/workflows/publish-to-pypi.yml but that does not run as CI and not very sure how you ask it to only run as packaging test but not upload anything. Anyways, you cannot have two publish workflows, so you need to delete this one if you are going to add the other one to CI.

nden · 2022-02-20T23:59:52Z

As Pey-Lian mentioned - add a pyproject.toml file. Then refactor __init__.py to define __version__ similar to other packages.

FDauphin

In setup.py, we should determine what information goes as PACKAGENAME, AUTHOR, etc.

In __init__.py, if we're going to use from __future__ that should be refactored. from .version is looking for a module named wfc3tools.version, which doesn't exist. Nadia mentioned refactoring __version__ like the other packages, but I don't know if that solves the problem.

pllim · 2022-02-24T20:24:12Z

@FDauphin , all the metadata should now be in setup.cfg. And __version__ is generated by setuptools_scm. You don't need from __future__.

pllim

(Just a quick review of that you have so far.)

Change package_name in setup.cfg to just name.

You don't need anything in setup.py besides invoking setuptools_scm to generate the version file. Move all the remaining metadata to appropriate sections in setup.cfg.

MANIFEST.in

pllim · 2022-02-24T20:24:53Z

pyproject.toml

+
+[tool.setuptools_scm]


Don't need this?

Suggested change

[tool.setuptools_scm]

roman and jwst both include this in this file, is it not necessary? what does including it here imply, that its needed for the setup?

setup.cfg

pllim · 2022-02-24T20:28:30Z

wfc3tools/__init__.py

+if sys.version_info < (3, 6):
+    raise ImportError("wfc3tools supports Python versions 3.6 and above.")  # pragma: no cover
+


Don't need this. Enforced by setting in setup.cfg.

Suggested change

if sys.version_info < (3, 6):

raise ImportError("wfc3tools supports Python versions 3.6 and above.") # pragma: no cover

pllim · 2022-02-24T20:30:25Z

wfc3tools/__init__.py

+    raise ImportError("wfc3tools supports Python versions 3.6 and above.")  # pragma: no cover
+
+try:
+    __version__ = get_distribution(__name__).version


This is overkill. If setuptools_scm worked correctly, you can do from .version import version as __version__ directly.

@nden commented to say I should add version in init

i've remove the minimum version check in this file, since setup.cfg should be enforcing this (i think?). i also did not add a version here because even without declaring it here, I am still able to access wfc3tools.version, so I assume this is done by setuptools

cshanahan1 · 2022-02-25T16:40:02Z

I'm removing doc related extras in setup.cfg. I'm going to open up another PR with all the doc stuff. Theres a lot in there and I want to distill it down to what is actually required.

nden · 2022-02-25T22:42:28Z

setup.py

-        import pytest
-        errno = pytest.main(self.test_args)
-        sys.exit(errno)
+NAME = 'wfc3tools'


I think this is not needed. It's defined in setup.cfg

nden

I installed the package and found a few minor issues. After fixing them I think this is good to go.

nden · 2022-02-26T12:45:26Z

wfc3tools/util.py

@@ -2,7 +2,7 @@

 from __future__ import print_function, division
 import warnings
-from .version import __version__
+importlib.metadata import version


this is not going to work, missing from

nden · 2022-02-26T12:55:18Z

setup.cfg

 author = STScI
 author_email = help@stsci.edu
 license = BSD
-edit_on_github = False
 github_project = spacetelescope/wfc3tools
 description-file = README.rst


When I install the package I see a Warning

UserWarning: Usage of dash-separated 'description-file' will not be supported in future versions. Please use the underscore name 'description_file' instead

setup.cfg

nden · 2022-02-26T13:10:29Z

MANIFEST.in

 include README.rst
 include CHANGES.rst
+include setup.cfg
+include LICENSE


The actual name is LICENSE.txt

pllim

Close...

If you want to remove the TEAL doc, should at least move them to proper RTD sections so we don't lose the info.

Also, I don't see any CI set up to test the packaging.

setup.cfg

pllim · 2022-03-03T18:52:39Z

setup.py

+    use_scm_version=True,
+    setup_requires=['setuptools_scm'],
+    packages=find_packages())


Suggested change

use_scm_version=True,

setup_requires=['setuptools_scm'],

packages=find_packages())

use_scm_version={'write_to': 'wfc3tools/version.py'})

done, but just curious why 'packages' isn't needed?

pllim · 2022-03-03T18:52:50Z

setup.py

@@ -1,82 +1,7 @@
 #!/usr/bin/env python
-import os
-import subprocess
-import sys
 from setuptools import setup, find_packages


Suggested change

from setuptools import setup, find_packages

from setuptools import setup

pllim · 2022-03-03T18:53:58Z

wfc3tools/__init__.py

-    teal.print_tasknames(__name__, os.path.dirname(__file__))
-except (ImportError, UnboundLocalError):
-    print("Teal not installed, gui param editing disabled")
+__version__ = version('wfc3tools')


Suggested change

__version__ = version('wfc3tools')

try:

from .version import version as __version__

except ImportError:

__version__ = ''

wfc3tools/__init__.py

pllim · 2022-03-03T18:54:45Z

wfc3tools/calwf3.py


 # STSCI
 from stsci.tools import parseinput
 from .util import error_code

-try:
-    from stsci.tools import teal


Is WFC3 team ok with removing TEAL support? This seems out of scope.

Back in September, I asked Michele De La Pena (might be helpful to ask her review as well) if we could remove TEAL support, but she said she was unsure. I am in support of removing it because I do not know in any instances where TEAL is the preferred method of using wfc3tools.

I agree that TEAL has probably outlived its usefulness, but someone should do the due diligence to make sure all the doc you are deleting are already reflected in RTD docs. If not, they have to be ported. Otherwise, you might lose precious info. Up to you.

the info in the .help files is going to be added back in by fred's PR

@cshanahan1 is correct; the help files info is added as docstrings in PEP 8 style and is added in the RTD. However, in the new RTD I initially removed all TEAL dependency. I suppose it would be best to add a sentence or two on the main RTD page that TEAL dependency was removed and why so.

FDauphin

I was able to clone this branch, install wfc3tools, and import with no issues. With regards to TEAL, I asked Michele De La Pena if removing it is the right decision (which I think it is).

pllim · 2022-03-03T19:16:21Z

In terms of packaging, testing source install isn't enough. You want to make sure you are able to build sdist and wheel, and then install those and get a full package back. I recently had to help debug another package that accidentally released an empty wheel.

nden · 2022-03-03T19:28:55Z

I built and tested a source package and a wheel. I can confirm they work.

nden · 2022-03-03T19:42:20Z

@pllim The idea for this PR is to make the build scripts up to date and release so we can start putting together and testing a larger HST distribution. The documentation will come in the next PR, followed by a new release but meanwhile we can work on the larger HST distribution. Is there anything else you would like to see done for this PR?

pllim

For completeness, maybe remove the following from .gitignore:

RELIC-INFO
relic/

Otherwise, LGTM. Thanks!

cshanahan1 · 2022-03-03T19:57:48Z

ok thanks everyone! im going to merge this now

FDauphin · 2022-03-03T20:04:33Z

@cshanahan1 thanks for your work on this! Michele followed up and said as long as drizzlepac (which still uses TEAL) isn't used in wfc3tools, we can remove TEAL. wfc3tools doesn't use drizzlepac so removing TEAL was the right move.

cshanahan1 requested review from nden, pllim and FDauphin February 17, 2022 03:41

pllim requested changes Feb 18, 2022

View reviewed changes

FDauphin suggested changes Feb 24, 2022

View reviewed changes

pllim reviewed Feb 24, 2022

View reviewed changes

nden reviewed Feb 25, 2022

View reviewed changes

nden requested changes Feb 26, 2022

View reviewed changes

cshanahan1 force-pushed the setup_changes branch from aa1a041 to 9056293 Compare March 3, 2022 14:51

cshanahan1 changed the title ~~changes to setup~~ changes to setup, remove teal Mar 3, 2022

nden approved these changes Mar 3, 2022

View reviewed changes

nden requested a review from pllim March 3, 2022 18:41

pllim requested changes Mar 3, 2022

View reviewed changes

FDauphin approved these changes Mar 3, 2022

View reviewed changes

pllim approved these changes Mar 3, 2022

View reviewed changes

cshanahan1 force-pushed the setup_changes branch from d99a1db to f7d7723 Compare March 3, 2022 19:55

changes to setup, remove teal

7e6971c

cshanahan1 force-pushed the setup_changes branch from f7d7723 to 7e6971c Compare March 3, 2022 19:57

cshanahan1 merged commit 87c567c into spacetelescope:master Mar 3, 2022

		if sys.version_info < (3, 6):
		raise ImportError("wfc3tools supports Python versions 3.6 and above.") # pragma: no cover

	from setuptools import setup, find_packages
	from setuptools import setup

-__version__ = version('wfc3tools')
+try:
+    from .version import version as __version__
+except ImportError:
+    __version__ = ''

changes to setup, remove teal #63

changes to setup, remove teal #63

Conversation

cshanahan1 commented Feb 17, 2022 • edited Loading

pllim left a comment

Choose a reason for hiding this comment

pllim commented Feb 18, 2022

nden commented Feb 20, 2022

FDauphin left a comment

Choose a reason for hiding this comment

pllim commented Feb 24, 2022

pllim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cshanahan1 commented Feb 25, 2022

Choose a reason for hiding this comment

nden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FDauphin Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

FDauphin left a comment • edited Loading

Choose a reason for hiding this comment

pllim commented Mar 3, 2022

nden commented Mar 3, 2022

nden commented Mar 3, 2022

pllim left a comment

Choose a reason for hiding this comment

cshanahan1 commented Mar 3, 2022

FDauphin commented Mar 3, 2022

cshanahan1 commented Feb 17, 2022 •

edited

Loading

FDauphin Mar 3, 2022 •

edited

Loading

FDauphin left a comment •

edited

Loading