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

Incorrect MIN Python Version in README #957

Open
NimaSarajpoor opened this issue Feb 11, 2024 · 11 comments
Open

Incorrect MIN Python Version in README #957

NimaSarajpoor opened this issue Feb 11, 2024 · 11 comments

Comments

@NimaSarajpoor
Copy link
Collaborator

The MIN Python version mentioned In README.rst is outdated (see the following line)

STUMPY supports [Python 3.7+](https://python3statement.org/) and, due to ...

Is it okay to just change it to 3.8+? Because STUMPY has not tested the support for python 3.12 yet in .github/workflows/github-actions.yml.

@seanlaw
Copy link
Contributor

seanlaw commented Feb 12, 2024

Hmm, that's a good point. It's probably a good idea to update it. To some extent, STUMPY technically still supports Python 3.7 but only for older STUMPY versions. However, this is confusing and it is better to be consistent and update everything to 3.8.

We should add "f) README.md" to the comments of the pypi.sh file of "things-to-do":

stumpy/pypi.sh

Lines 7 to 12 in 3da1500

# 5. Bump minimum versions and dependencies
# a) setup.py
# b) requirements.txt
# c) environment.yml
# d) .github/worflows/github-actions.yml
# e) recipes/meta.yaml in conda-feedstock


Because STUMPY has not tested the support for python 3.12 yet in .github/workflows/github-actions.yml

Indeed. I have added #958 (and corresponding STUMPY v1.12.1/v.1.13 Milestone) to track this.

@joehiggi1758
Copy link

Hey All - hope you're having a wonderful day!

Would it be alright if I take this one? I'm new to OSS contributions, but want to help!

@seanlaw
Copy link
Contributor

seanlaw commented May 21, 2024

@joehiggi1758 Thank you for your interest in contributing to STUMPY. Yes, please feel free to take this one! It looks like we need:

  1. Update the Python version listed in the README.md file
  2. Add a note to the pypi.sh file for things to update/change when we upload a new version to PyPI
  3. Check if we have 3.7 hardcoded anywhere else in our docs/tutorials?

Please do not hesitate to ask any questions and take a look at our Contributors Guide for a good place to start (that document might need some love too as it might be outdated).

@joehiggi1758
Copy link

@seanlaw thanks so much for the opportunity to help, I'm on it!

  1. Done!
  2. Can you expand a bit here?
  3. Currently working on!

Will try to knock this one out by end of week!

@seanlaw
Copy link
Contributor

seanlaw commented May 22, 2024

Can you expand a bit here?

So, the pypi.sh file is a simple helper script that we use to help us upload the latest package version to PyPI. Since this uploading task is only performed once in a while, we also store "last minute steps" (as comments) in this file to remind us of the additional things that we need to check off our list BEFORE we upload to PyPI. In this particular case, we should add f) README.md to the comments of the pypi.sh file of "things-to-do":

# 5. Bump minimum versions and dependencies
#    a) setup.py
#    b) requirements.txt
#    c) environment.yml
#    d) .github/worflows/github-actions.yml
#    e) recipes/meta.yaml in conda-feedstock
#    f) README.md
# 6. Commit all above changes as the latest version number and push

Currently, the list ends at 5. e) but we should add a 5. f) as shown above so that we are reminded to update the information. Let me know if that makes sense or please seek further clarification.

Actually, I just realized that we no longer have a setup.py file (since we migrated over to using a pyproject.toml file instead for building our package). So, would you mind replacing the above with:

# 5. Bump minimum versions and dependencies
#    a) pyproject.toml
#    b) requirements.txt
#    c) environment.yml
#    d) .github/worflows/github-actions.yml
#    e) recipes/meta.yaml in conda-feedstock
#    f) README.md

@seanlaw
Copy link
Contributor

seanlaw commented May 22, 2024

@joehiggi1758 Once this is done then somebody can work on issue #973, which is somewhat related to this

@joehiggi1758
Copy link

@seanlaw sounds great! I'd be happy to snag #973 if nobody else has (after I close this one out)!

Apologies for the likely naïve question - but I can't seem to get my local repository to pass the full set of tests in "test.sh" and keep getting the following.

image

I've followed the Stumpy setup docs exactly and have tried everything I can think of - any direction on how to clear would be much appreciated!

@seanlaw
Copy link
Contributor

seanlaw commented May 23, 2024

Apologies for the likely naïve question

Not naive at all! Let's try to figure this out together and maybe update the Contributors Guide if we're missing anything obvious. Sometimes, it's a matter of figuring out how things work on different development environments.

but I can't seem to get my local repository to pass the full set of tests in "test.sh" and keep getting the following.
I've followed the Stumpy setup docs exactly and have tried everything I can think of - any direction on how to clear would be much appreciated!

Hmmm, a couple of things:

  1. I don't use .venv but it almost seems like you aren't running the tests in the stumpy root directory? I think this because it says "2383 files would be reformatted, 560 files would be left unchanged" but stumpy doesn't have that many files!
  2. Usually when you see the "broken heart" emoji, it suggests that the code formatting is not adherent to [black code formatting](I've followed the Stumpy setup docs exactly and have tried everything I can think of - any direction on how to clear would be much appreciated!). When successful, running black . in the correct directory would result in

All done! ✨ 🍰 ✨
86 files left unchanged.

In other words, STUMPY should only have around 86 files that would be checked by black

@joehiggi1758
Copy link

joehiggi1758 commented May 24, 2024

@seanlaw thank you very much for the direction here - it was super helpful!

I figured out the issue - and I do believe this warrants an update to the 'Contributing Guide' and perhaps an update to 'test.sh'. I had cloned STUMPY locally to my installation of MS VS Code (not using Anaconda), and when I would run 'test.sh' in the STUMPY root directory, it would actually look through my virtual environment (which I had called .venv) and run tests on all files contained, including isolated package installations, hence why my file count was insanely high ~2900 in the above output.

I switched to using Anaconda and was able to get everything working - I have submitted a pull request for the above! I'm open to any and all feedback as I'm eager to learn, please let me know how I can improve moving forward!

Once this issue closes, I will happily pick up #973 if it's still open!

@seanlaw
Copy link
Contributor

seanlaw commented May 24, 2024

I figured out the issue - and I do believe this warrants an update to the 'Contributing Guide' and perhaps an update to 'test.sh'. I had cloned STUMPY locally to my installation of MS VS Code (not using Anaconda), and when I would run 'test.sh' in the STUMPY root directory, it would actually look through my virtual environment (which I had called .venv) and run tests on all files contained, including isolated package installations, hence why my file count was insanely high ~2900 in the above output.

Hmm, I've not used .venv virtual environments before but, typically, test.sh would simply execute in the current working directory (i.e., the root stumpy directory). There's nothing special about test.sh so it wouldn't go looking in random places. In VSCode, within the VSCode terminal, what happens when you type pwd? Maybe you still need to navigate to the root stumpy directory within your terminal as well before executing the test.sh file? How are you running test.sh? From the VSCode terminal or from the VSCode GUI?

FYI, I am trying to determine if the "problem" is actually coming from VSCode or from .venv. Also, maybe we should run setup.sh ci instead of setup.sh (in the Contributor guide) as the former will try to install the dependencies for unit testing/development as well. I've added issue #975 to track any changes needed for the Contributor Guide.

Once this issue closes, I will happily pick up #973 if it's still open!

Sounds good! Let's leave #973 last (as it is more complex) and handle this issue as well as #975 first.

@seanlaw
Copy link
Contributor

seanlaw commented May 24, 2024

thank you very much for the direction here - it was super helpful!

No, thank YOU! Sometimes, contributing (to Python packages) can be front loaded with frustrating issues like this that have nothing to do with the actual Python coding and many people quit due to how daunting it feels. So, your persistence is greatly appreciated!

I'm open to any and all feedback as I'm eager to learn, please let me know how I can improve moving forward!

@joehiggi1758 You have the right mindset so let's learn and make progress together! :)

joehiggi1758 added a commit to joehiggi1758/stumpy that referenced this issue May 28, 2024
…UMPY supports Python 3.8+ updated pypi.sh to reference step a) pyproject.toml instead of the decomissioned setup.py and ultimately scanned the full repository for any listings of outdated references to STUMPY supporting Python 3.7 (I was unable to find any)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants