Skip to content

DEP: add pytest-skip-slow as a dependency and remove redundant definitions#79

Merged
pllim merged 2 commits into
astropy:mainfrom
neutrinoceros:dep/pytest-skip-slow
Jun 10, 2026
Merged

DEP: add pytest-skip-slow as a dependency and remove redundant definitions#79
pllim merged 2 commits into
astropy:mainfrom
neutrinoceros:dep/pytest-skip-slow

Conversation

@neutrinoceros

Copy link
Copy Markdown
Contributor

close #73
companion to astropy/astropy#18784

pllim
pllim previously requested changes May 28, 2026

@pllim pllim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR seems to remove hugemem support silently even though it is used: https://github.com/search?q=repo%3Aastropy%2Fastropy%20hugemem&type=code

This change is pretty significant but no change log. I think we need one.

We are switching from a package we have total control over to a package with a single point of failure, weakening the supply chain security. I don't see how this is an improvement overall.

@neutrinoceros

Copy link
Copy Markdown
Contributor Author

hugemem isn't suppressed, pytest-skip-slow also defines it.
About the package being outside our control: I anticipated this counter argument, and already proposed moving the repo over the astrophysics org, which the author agreed to.

@neutrinoceros

Copy link
Copy Markdown
Contributor Author

the change should actually be completely invisible to end users. Are you sure this warrants a change log entry ?

@pllim

pllim commented May 28, 2026

Copy link
Copy Markdown
Member

astrophysics org

Is this our org or something else? I would propose we put this effort on hold until after that org move has happened.

@pllim

pllim commented May 28, 2026

Copy link
Copy Markdown
Member

completely invisible to end users

End-users for pytest-astropy are going to care, so yes, we need a change log.

@neutrinoceros

Copy link
Copy Markdown
Contributor Author

Sorry, autocorrect got in the way. I indeed meant astropy

@neutrinoceros neutrinoceros force-pushed the dep/pytest-skip-slow branch from 05ef2e6 to b89ec80 Compare May 28, 2026 15:08
@neutrinoceros

Copy link
Copy Markdown
Contributor Author

changelog added

@Cadair

Cadair commented Jun 3, 2026

Copy link
Copy Markdown
Member

This package is a super trivial dep, so I don't really think we need to adopt it and I'm fine depending on it.

@hamogu

hamogu commented Jun 3, 2026

Copy link
Copy Markdown
Member

We discussed this in the dev-telecom today and we all agree that it's a good idea! Please merge ;-)

@pllim

pllim commented Jun 3, 2026

Copy link
Copy Markdown
Member

I thought we moving it to our org first? What happened?

@neutrinoceros

Copy link
Copy Markdown
Contributor Author

That's what I proposed over the telecon but participants preferred leaving it where it is.

@pllim

pllim commented Jun 3, 2026

Copy link
Copy Markdown
Member

participants preferred leaving it where it is

What are the reasonings for this preference? Can y'all please clarify? Thanks.

@neutrinoceros

Copy link
Copy Markdown
Contributor Author

To paraphrase @Cadair, it's just 50 lines of code. If it ever fails us, however unlikely, it'll always be possible to copy it back here.

For good measure and redundancy, I also offered Brian to have me as a co-maintainer.

@pllim

pllim commented Jun 3, 2026

Copy link
Copy Markdown
Member

In regards to package ownership, it is the supply chain and bus factor I worry about, not the SLOC.

@neutrinoceros

Copy link
Copy Markdown
Contributor Author

If you insist it's still possible to make the move. I didn't hear any strong opinions against it, just a preference for doing the easy thing.

@hamogu

hamogu commented Jun 3, 2026

Copy link
Copy Markdown
Member

The bus factor is addressed by "50 lines of code". If the maintainer suddenly leaves the package, we can still copy the code (or ask him to transfer as last act) and take it over at that point but why bother now.

Supply chain: We trust the maintainer now, and he'd be a maintainer if we transfer it, too, so I don't see any change. But for a package that changes infrequently and don't need urgent and automatic security fixes (this one and others similar to it) we may consider to pin the version to improve supply chain security.

@neutrinoceros

Copy link
Copy Markdown
Contributor Author

@pllim is any of your objections blocking against a clear majority ?

@neutrinoceros

Copy link
Copy Markdown
Contributor Author

(or in other words, is veto a thing in our decision process ?)

@pllim

pllim commented Jun 10, 2026

Copy link
Copy Markdown
Member

What is the maintenance status now and what is the path forward?

  • Are you an admin over there?
  • Are we ever going to move that package to our org?

@pllim

pllim commented Jun 10, 2026

Copy link
Copy Markdown
Member

I do not have veto power but I will be VERY annoyed if I have to deal with fallout from this decision.

@hamogu

hamogu commented Jun 10, 2026

Copy link
Copy Markdown
Member

Again, if problems ever arise, we can move or work the project at a later stage. It's a tiny project and won't take many hours of work to move/fork/copy if we ever need to.
@pllim What fallout are you worried about that would be avoided by moving to the astropy org now, and which could not be mitigated by a move/copy/fork later in a similar amount of work time later if needed?

@hamogu hamogu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid that we are getting into bike-shedding territory here: The number of lines written here and in #73 exceed the LOC in question by more than an order of magnitude.

One concern that we (and everybody else) is battling more and more is supply chain vulnerability, which increases simply with the number of package (large or small) that we depend on. That's not necessarily due to ill intent of the maintainers, their pypi accounts could be compromised or something like that.
Since this new dependency is expected to evolve very slowly or never, I ask to pin the dependency to a fixed version.

Yes, we will at some point forget to upgrade, but that's the price we pay for splitting one dependency into several.

@neutrinoceros neutrinoceros force-pushed the dep/pytest-skip-slow branch from b89ec80 to 9ce4ea3 Compare June 10, 2026 17:22
@neutrinoceros

Copy link
Copy Markdown
Contributor Author

done

Comment thread CHANGES.rst Outdated
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
@pllim pllim dismissed their stale review June 10, 2026 17:45

Outvoted

@pllim pllim merged commit 635554b into astropy:main Jun 10, 2026
1 check passed
@neutrinoceros neutrinoceros deleted the dep/pytest-skip-slow branch June 10, 2026 18:30
@bsipocz

bsipocz commented Jun 10, 2026

Copy link
Copy Markdown
Member

I'm afraid that we are getting into bike-shedding territory here: The number of lines written here and in #73 exceed the LOC in question by more than an order of magnitude.

Respectfully this PR and all the bikeshedding done was against the common consensus of #73 to just leave stuff alone. So it's kind of contradicting from both of you to push for these PRs and disucssion and then use the argument that the rest of us are bikeshedding. I do keep asking, you as the line manager as well, do you think it's a good use of astropy money to do busywork like this, isn't there a better way to use maintainer energy and funded work?

@bsipocz

bsipocz commented Jun 10, 2026

Copy link
Copy Markdown
Member

So yes, I feel and agree with @pllim #79 (comment), but apparently strongarming her as well as the common decision that was made a bit ago in #73 is the way to go...

@hamogu

hamogu commented Jun 11, 2026

Copy link
Copy Markdown
Member

@bsipocz and @pllim : You are free to disagree, but, as I said above, we discussed this in the dev telecon with six developers present and there was broad consensus for the approach taken here. My understanding is that that's exactly what the dev telecon is for: Discuss issues related to development, infrastructure, code design, etc.
I understand that not everyone can be present at that time and that significant discussion happens in GH, but if a consensus after discussion in the dev telecon doesn't count for anything, then I don't know why we have any discussions there in the first place.

So, while this is merged now, I want to write down for the record that this PR is, as @bsipocz said, "against the consensus in #73", but was approved by a broad consensus in the dev telecon after considering the comments in #73.

I will take this though to add the discussion topic "Should we continue to have dev telecoms?" to the next in-person meeting agenda this fall, since there is an obvious tension that the people discussion in issues are not the same people discussion in the telecon.

(And I didn't accuse "the rest of us" of bike shedding. I absolutely include me into that and specifically said "we reached" not "you reached"; my intend was to just state that, in my opinion, we have all together reached bike shedding territory. I'm also not the line manager for anyone (I don't know the list of current "managers", you would have to check with the finance committee. I'm not managing anyones contract with NumFOCUS/Astropy and haven't for over a year.) I also want to state, also for the record, that no astropy grant money has reached MIT yet, so my "line manager" can also not complain about me spending time here. I'm commenting as a volunteer.)

@bsipocz

bsipocz commented Jun 11, 2026

Copy link
Copy Markdown
Member

You are free to disagree, but, as I said above, we discussed this in the dev telecon with six developers present and there was broad consensus for the approach taken here. My understanding is that that's exactly what the dev telecon is for:

Then it's just not cool to keep pushing stuff and bike shedding in the dev telecon that has already reached broad consensus in the GH issue.

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.

Consider splitting the homebrewed plugin as it on lightweight package ?

5 participants