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.

Sign up for GitHub

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

Jump to bottom

sphobjinv-textconv #307

Closed
wants to merge 0 commits into from
Closed

sphobjinv-textconv #307

wants to merge 0 commits into from

Conversation

msftcangoblowm
Copy link

closes #295

The rebase was challenging. Previous PR was automatically withdrawn

@msftcangoblowm
Copy link
Author

During python setup, gh runner failings to download python 3.13 for linux and windows.
Checked the versions manifest and 3.13 for x64 architecture are available.

The runner says could be caused by an authentication issue.

versions-manifest json

##[error]Failed to download Python from the Github Actions python registry python-versions. Error: Error: Failed to download Python versions manifest from the Github Actions python registry python-versions. Invalid GitHub token could cause this.

##[error]Version spec 3.13 for architecture x64 did not match any version in Agent.ToolsDirectory.

This is the relevent entries from the versions manifest. (to save time searching thru it)

{
        "filename": "python-3.13.1-linux-20.04-x64.tar.gz",
        "arch": "x64",
        "platform": "linux",
        "platform_version": "20.04",
        "download_url": "https://github.com/actions/python-versions/releases/download/3.13.1-12154081405/python-3.13.1-linux-20.04-x64.tar.gz"
},
{
        "filename": "python-3.13.1-linux-22.04-x64.tar.gz",
        "arch": "x64",
        "platform": "linux",
        "platform_version": "22.04",
        "download_url": "https://github.com/actions/python-versions/releases/download/3.13.1-12154081405/python-3.13.1-linux-22.04-x64.tar.gz"
      },
{
        "filename": "python-3.13.1-linux-24.04-x64.tar.gz",
        "arch": "x64",
        "platform": "linux",
        "platform_version": "24.04",
        "download_url": "https://github.com/actions/python-versions/releases/download/3.13.1-12154081405/python-3.13.1-linux-24.04-x64.tar.gz"
      },

{
        "filename": "python-3.13.1-win32-x64.zip",
        "arch": "x64",
        "platform": "win32",
        "download_url": "https://github.com/actions/python-versions/releases/download/3.13.1-12154081405/python-3.13.1-win32-x64.zip"
      },

@bskinn
Copy link
Owner

bskinn commented Jan 2, 2025

That same config worked recently for a different pipeline, so I don't think it's a config or token problem. More likely, there was just a transient issue with the GitHub Python repository. I'll trigger a retry; if it still fails, then let's just remove 3.13 from azure-pipelines.yml and not worry about it.

I'm working on an overhaul of the CI (#306) that will migrate everything over to GitHub Actions anyways, which has 3.13 natively available.

@bskinn
Copy link
Owner

bskinn commented Jan 2, 2025

No, still fails with the same errors. Just remove 3.13 from the Azure DevOps matrix.

@msftcangoblowm
Copy link
Author

msftcangoblowm commented Jan 5, 2025 via email

@msftcangoblowm
Copy link
Author

msftcangoblowm commented Jan 5, 2025 via email

@bskinn
Copy link
Owner

bskinn commented Jan 5, 2025

Mm, I didn't think about the rebase being such a problem... merging main into your working branch would have probably worked equally well and been a lot less awful. Sorry about that!!

bskinn
bskinn reviewed Jan 5, 2025
View reviewed changes
Copy link
Owner

@bskinn bskinn left a comment

Choose a reason for hiding this comment

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

Duplicating the review comments from #301

@msftcangoblowm
Copy link
Author

After a quick sync, the latest commit addresses the issues you brought up, besides # pragma: no cover comments.

@bskinn bskinn added type: enhancement ✨ Something to add area: cli 💻 function: convert 🧪 bump: minor 🐒 Backward-compatible feature adds (x.y.0) labels Jan 9, 2025
bskinn
bskinn reviewed Jan 9, 2025
View reviewed changes
bskinn
bskinn reviewed Jan 12, 2025
View reviewed changes
Copy link
Owner

@bskinn bskinn left a comment

Choose a reason for hiding this comment

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

Several more comments across the PR. Still haven't had a chance to dig deeply into the new code itself, or the docs. Will get there.

@msftcangoblowm
Copy link
Author

Please rerun azure pipelines. Getting remotedisconnect and ssl errors

bskinn
bskinn reviewed Jan 15, 2025
View reviewed changes
Copy link
Owner

@bskinn bskinn left a comment

Choose a reason for hiding this comment

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

Thanks for your patience as I nibble-review on this!

tox.ini Outdated
Comment on lines 134 to 135
# cd .tox; PYTHONPATH=../ tox --root=.. -c ../tox.ini \
# -e py39-sphx_latest-attrs_latest-jsch_latest --workdir=.; cd - &>/dev/null
Copy link
Owner

Choose a reason for hiding this comment

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

Are these commented-out lines still needed?

Copy link
Author

Choose a reason for hiding this comment

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

Instantly forget complicated commands.

For collaborators, somewhere these commands should be documented. I prefer in context, so zero time is wasted recreating them.

non-tox commands should be in a Makefile.

pytest commands within the respective test module.

Have always written, just the complicated, tox commands within the tox file.

This particular command is how to invoke for pyenv users. The active .python-version file differs for: project base (one py version) and the .tox folder (many py versions)

Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand - why are you running the Python from tox environment instead of just executing pytest from the root of the repository with an activated and configured virtual environment, as per CONTRIBUTING.md?

As noted there, tox on this project isn't really intended for contributor use. The matrix of test envs in tox.ini is for my benefit, as part of my release process.

If it works to create a new virtual environment with a pyenv-managed Python, activate that virtual environment, pip-install requirements-dev.txt into it, and run pytest, then that should be the workflow.

I'm going to take a lot of persuading to keep this in here. If this invocation really is important to document, it should be in CONTRIBUTING.md and include more context and probably more instructions.

@bskinn
Copy link
Owner

bskinn commented Feb 10, 2025

Hey, @msftcangoblowm -- sorry for the delay, thanks for your patience.

First thing -- I've merged the rework of the CI. Please delete .github/workflows/ci-tests.yml and all of the Azure Pipelines YAML files, and then merge main into your working branch again. I think that should work cleanly and resolve the merge conflict.

@bskinn
Copy link
Owner

bskinn commented Feb 10, 2025

Code review wise -- I think it'll be best to work on the code first, and then follow with the docs once the code is settled.

First, high-level, I'm not sure what the value is for having so much re-implemented in the new textconv code. I don't really want to have two spellings for a bunch of stuff that's already in the original sphobjinv command. I was picturing sphobjinv-textconv to be a very narrow, single-purpose utility:

  • Only reading an .inv from disk (so, no --url support)
  • No --expand or --contract options
  • No 'adapt to survive' logic, really - you provide it a path to a file, it converts it to plaintext on stdout with no other modifications, and that's it.

It also seems like there's a lot of unnecessary/irrelevant logic in .core_textconv.main().

In sum -- I think all of the new functional code should be much slimmer. Is that workable, to make a trim-down refactoring pass on everything?

Second, related -- I'm having a hard time understanding the purpose/function of print_stderr_2 and _wrap_inv_stdin. Are they leftovers from the attempt to include live git textconv testing in the test suite? If they're not specifically needed for any remaining tests to function correctly, they should definitely be removed.

(If they are needed for some remaining tests to function correctly, please make an attempt to revise the tests so that they can successfully use the original print_stderr().)

Thanks!

@msftcangoblowm
Copy link
Author

msftcangoblowm commented Feb 10, 2025 via email

@bskinn
Copy link
Owner

bskinn commented Feb 12, 2025

Since sphobjinv supports three OS using a variety of py interpreters, before submitting a commit, been testing all that i can. To be confident have reduced the surface area of possible issues. Or at least what i can, since don't have Windows and MacOS VMs to test everything locally.

I appreciate your desire to have everything be tested cross-py and cross-platform. However, I see that as being the purpose of the CI. My contributor experience goal is for the direct efforts of contributors to only involve testing with whatever Python and platform they happen to be using; or, even not running tests locally at all, if they wish.

The entire point of tox is to test multiple py interpreters in isolated environments.

CI also provides this, with much lower effort by contributors.

Testing should occur per commit.

Some testing per commit, yes. Exhaustive testing per commit, no, I disagree---at least not for sphobjinv, as a pure-Python package. If it included, e.g., a compiled C or Rust extension I would lean more toward thorough per-commit testing; but as it is, it seems unnecessary.

... unnecessary, actually, to the point where the CI overhaul of #306 actually made it possible for contributors to only have two test cases run on draft PRs, currently Python 3.12 on Ubuntu and Windows. Only when a PR is marked as 'ready' does a full text matrix run, plus linting. My hope is that this will allow for more rapid initial development for myself and for contributors; and, if a contributor wants the full test matrix from the outset, they have the option of leaving the PR in 'ready' state.

that would mean avoidable errors aren't being tested and found, ...

But the CI will catch them. And, it's been exceedingly rare, actually, that I've had a piece of work function fine on one actively supported version of Python, but fail on another---and similarly with the various runtime dependencies. Cross-platform issues have occurred more often, but they're pretty much a non-issue now that I've given up on doctesting the shell examples in the README.

... causing lots of additional commits.

Commits are cheap, I don't have any problem at all with contributions that contain many commits. I break my work up into lots of commits already, including lots of 'whoops, fix X' commits from mistakes I've made. I'm not at all concerned about a sprawling Git history, at least in sphobjinv. I actually prefer it, because a long chain of granular commits makes it much easier to follow the details of the arc of the development process.

If sphobjinv had a large, active, async dev team, with many feature branches active at once, I might feel differently. But it doesn't, so I have the luxury of not worrying about it.

There is the additional issue of azure-pipelines.yml being flaky. So lets say a commit has errors. Am i supposed to wait for you each time to click a button to rerun the pipeline? Especially when it's an issue i coulda caught locally.

Very good point. I actually already added some pytest-retry flaky marks to all of the --nonloc tests as part of #306, to try to significantly reduce these spurious test failures.

The inability for the PR author to rerun failed pipelines is quite annoying, but I understand it from a security perspective. (Maybe GitHub Actions makes it easier/possible for PR authors to trigger retries? Not sure.) In any event, the usual approach to this is to make another, trivial commit to kick off a new pipeline. Kind of janky, but it is what it is.

Normally, the test and release processes are separate. The release process is normally triggered upon setting a tag and pushing the tag.

I misspoke. I meant to say that I use the tox matrix as part of my pre-release process.

All of this said, you're welcome to use tox if you like, and if this invocation is what it takes to make tox work with pyenv Pythons, then it's worth documenting. (I'd be surprised if it's necessary, though -- I would expect pyenv to expose its various Python 3.x on PATH as the respective python3.# executables; and if it doesn't, that makes me glad I never tried to use it.)

But, in order to keep it as part of the PR, it would need to go in CONTRIBUTING.md, and would need to be documented more clearly with an explanatory writeup.

Especially if this is the only thing holding up the PR merge.

No, it's unfortunately not the only thing preventing merge. Per my other comment, both the code and the docs will need significant work before the PR is merge-ready. High-level, there is just too much of both code and docs; both will need trimming and revision. And, I'm still leaning against uptaking any instructions for setting up a Git textconv into the repo... I don't really want that maintenance burden.

@msftcangoblowm
Copy link
Author

msftcangoblowm commented Feb 13, 2025 via email

@bskinn bskinn mentioned this pull request Feb 14, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bskinn bskinn bskinn left review comments

Assignees
No one assigned
Labels
area: cli 💻 bump: minor 🐒 Backward-compatible feature adds (x.y.0) function: convert 🧪 type: enhancement ✨ Something to add
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

act as textconv so can git diff inventory
2 participants
@msftcangoblowm @bskinn