Skip to content

TST: double job to take args like single run#17743

Closed
pllim wants to merge 2 commits into
astropy:mainfrom
pllim:double-trouble-the-sequel
Closed

TST: double job to take args like single run#17743
pllim wants to merge 2 commits into
astropy:mainfrom
pllim:double-trouble-the-sequel

Conversation

@pllim

@pllim pllim commented Feb 7, 2025

Copy link
Copy Markdown
Member

Description

This pull request is a direct follow-up of #17732 (comment)

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@pllim pllim added this to the v7.0.2 milestone Feb 7, 2025
@github-actions

github-actions Bot commented Feb 7, 2025

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@github-actions

This comment was marked as outdated.

Sign in to view
@pllim pllim mentioned this pull request Feb 7, 2025
Merged
1 task
@pllim pllim force-pushed the double-trouble-the-sequel branch from 6358984 to 8c47362 Compare February 7, 2025 22:48
@pllim

pllim commented Feb 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

Okay.. When I throw in docs and -n=4, there are 3 failures. I need to ponder... Or if you have any ideas, @mhvk or @neutrinoceros ?

______________________________ test_future_altaz _______________________________
...
    def test_future_altaz():
...
        # check that these message(s) appear among any other warnings
>       with (
            pytest.warns(erfa.core.ErfaWarning),
            pytest.warns(
                AstropyWarning,
                match="Tried to get polar motions for times after IERS data is valid.*",
            ),
            iers.conf.set_temp("auto_max_age", None),
        ):
E       astropy.utils.exceptions.AstropyWarning: (some) times are outside of range covered by IERS table...

astropy/coordinates/tests/test_iau_fullstack.py:199: AstropyWarning

_____________________________ test_io_ascii_write ______________________________
...
    def test_io_ascii_write():
        """
        Test that table with mixin column can be written by io.ascii for
        every pure Python writer.  No validation of the output is done,
        this just confirms no exceptions.
        """
        from astropy.io.ascii.connect import _get_connectors_table
    
        t = QTable(MIXIN_COLS)
        for fmt in _get_connectors_table():
            if fmt["Write"] and ".fast_" not in fmt["Format"]:
                out = StringIO()
>               t.write(out, format=fmt["Format"])

astropy/table/tests/test_mixin.py:123: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
astropy/table/connect.py:130: in __call__
    self.registry.write(instance, *args, **kwargs)
astropy/io/registry/core.py:385: in write
    writer = self.get_writer(format, data.__class__)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <astropy.io.registry.core.UnifiedIORegistry object at 0xfff09e776b90>
data_format = np.str_('ascii.custom_commented_header')
data_class = <class 'astropy.table.table.QTable'>

    def get_writer(self, data_format, data_class):
...
        format_table_str = self._get_format_table_str(data_class, "Write")
>       raise IORegistryError(
            f"No writer defined for format '{data_format}' and class"
            f" '{data_class.__name__}'.\n\nThe available formats"
            f" are:\n\n{format_table_str}"
        )
E       astropy.io.registry.base.IORegistryError: No writer defined for format 'ascii.custom_commented_header' and class 'QTable'.
...
astropy/io/registry/core.py:337: IORegistryError

_________________________ [doctest] equivalencies.rst __________________________
...
664 However, when passing the spectral equivalency, you can see there are
665 all kinds of things that ``Hz`` can be converted to::
666 
667   >>> u.Hz.find_equivalent_units(equivalencies=u.spectral())
Differences (unified diff with -expected +actual):
    @@ -1,23 +1,33 @@
    -Primary name | Unit definition        | Aliases
    +  Primary name | Unit definition        | Aliases                           
     [
    -  AU           | 1.49598e+11 m          | au, astronomical_unit            ,
    -  Angstrom     | 1e-10 m                | AA, angstrom                     ,
    -  Bq           | 1 / s                  | becquerel                        ,
    -  Ci           | 3.7e+10 / s            | curie                            ,
    -  Hz           | 1 / s                  | Hertz, hertz                     ,
    -  J            | m2 kg / s2             | Joule, joule                     ,
    -  Ry           | 2.17987e-18 m2 kg / s2 | rydberg                          ,
    -  cm           | 0.01 m                 | centimeter                       ,
    -  eV           | 1.60218e-19 m2 kg / s2 | electronvolt                     ,
    -  earthRad     | 6.3781e+06 m           | R_earth, Rearth                  ,
    -  erg          | 1e-07 m2 kg / s2       |                                  ,
    -  foe          | 1e+44 m2 kg / s2       | Bethe, bethe                     ,
    -  jupiterRad   | 7.1492e+07 m           | R_jup, Rjup, R_jupiter, Rjupiter ,
    -  k            | 100 / m                | Kayser, kayser                   ,
    -  lsec         | 2.99792e+08 m          | lightsecond                      ,
    -  lyr          | 9.46073e+15 m          | lightyear                        ,
    -  m            | irreducible            | meter                            ,
    -  micron       | 1e-06 m                |                                  ,
    -  pc           | 3.08568e+16 m          | parsec                           ,
    -  solRad       | 6.957e+08 m            | R_sun, Rsun                      ,
    +  AU           | 1.49598e+11 m          | au, astronomical_unit              ,
    +  Angstrom     | 1e-10 m                | AA, angstrom                       ,
    +  BTU          | 1055.06 m2 kg / s2     | btu                                ,
    +  Bq           | 1 / s                  | becquerel                          ,
    +  Ci           | 3.7e+10 / s            | curie                              ,
    +  Hz           | 1 / s                  | Hertz, hertz                       ,
    +  J            | m2 kg / s2             | Joule, joule                       ,
    +  Ry           | 2.17987e-18 m2 kg / s2 | rydberg                            ,
    +  cal          | 4.184 m2 kg / s2       | calorie                            ,
    +  cm           | 0.01 m                 | centimeter                         ,
    +  eV           | 1.60218e-19 m2 kg / s2 | electronvolt                       ,
    +  earthRad     | 6.3781e+06 m           | R_earth, Rearth                    ,
    +  erg          | 1e-07 m2 kg / s2       |                                    ,
    +  foe          | 1e+44 m2 kg / s2       | Bethe, bethe                       ,
    +  ft           | 0.3048 m               | foot                               ,
    +  fur          | 201.168 m              | furlong                            ,
    +  inch         | 0.0254 m               |                                    ,
    +  jupiterRad   | 7.1492e+07 m           | R_jup, Rjup, R_jupiter, Rjupiter   ,
    +  k            | 100 / m                | Kayser, kayser                     ,
    +  kcal         | 4184 m2 kg / s2        | Cal, Calorie, kilocal, kilocalorie ,
    +  lsec         | 2.99792e+08 m          | lightsecond                        ,
    +  lyr          | 9.46073e+15 m          | lightyear                          ,
    +  m            | irreducible            | meter                              ,
    +  mi           | 1609.34 m              | mile                               ,
    +  micron       | 1e-06 m                |                                    ,
    +  mil          | 2.54e-05 m             | thou                               ,
    +  nmi          | 1852 m                 | nauticalmile, NM                   ,
    +  pc           | 3.08568e+16 m          | parsec                             ,
    +  solRad       | 6.957e+08 m            | R_sun, Rsun                        ,
    +  yd           | 0.9144 m               | yard                               ,
     ]

docs/units/equivalencies.rst:667: DocTestFailure

@pllim

pllim commented Feb 7, 2025

Copy link
Copy Markdown
Member Author

Maaaybe we don't need to ensure that docs folder can be run twice? My original beef was that doctest was not run in astropy.test() but now that we decided to get rid of it eventually, maybe I can just let that go...

The first 2 might be because of -n=4 since it is not from docs? Is it a good idea to mix up double run with parallel mode?

@mhvk

mhvk commented Feb 8, 2025

Copy link
Copy Markdown
Contributor

Agreed that we don't have to run the doctests twice - our goal for those is just to ensure a user doesn't stumble on a problem while trying things out.

The io failure, though, looks like it is hitting something real...

@pllim pllim force-pushed the double-trouble-the-sequel branch from 8c47362 to 939a127 Compare February 8, 2025 02:46
@pllim

pllim commented Feb 8, 2025

Copy link
Copy Markdown
Member Author

Huh... Removing the "docs" folder made the failures disappear. Perhaps including narrative docs polluted the state of the tests...

@pllim pllim marked this pull request as ready for review February 8, 2025 03:05
@pllim pllim changed the title TST: double job to run docs too and take args like single run TST: double job to take args like single run Feb 8, 2025
@pllim pllim requested review from mhvk and neutrinoceros February 8, 2025 03:07
@mhvk mhvk mentioned this pull request Feb 9, 2025
Merged
@mhvk

mhvk commented Feb 9, 2025

Copy link
Copy Markdown
Contributor

Ended up not liking that the doctests had side effects, so tried to fix them at #17747 - and added your first commit to see whether that indeed solves things.

@pllim pllim deleted the double-trouble-the-sequel branch February 10, 2025 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@neutrinoceros neutrinoceros Awaiting requested review from neutrinoceros neutrinoceros is a code owner
@mhvk mhvk Awaiting requested review from mhvk

Assignees

No one assigned

Labels

no-changelog-entry-needed testing

Projects

None yet

Milestone

v7.0.2

Development

Successfully merging this pull request may close these issues.

2 participants

@pllim @mhvk