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

Update distributions to Python 3. #18

Merged
merged 3 commits into from
Feb 21, 2024

Conversation

emilyfertig
Copy link

All unit tests pass, except those that depend on goftests, which is not yet updated to py3.

I had to make some hacky changes to setup.py to get this running (e.g. hard-coded local file paths), which aren't included in this PR. If someone wants to update setup.py, I'm happy to share what I did, if it's useful.

@jfinkels
Copy link

I updated goftests so that it should work with recent versions of numpy now, and there is a CI test for Python 3.10 posterior/goftests#24

Copy link
Member

@fritzo fritzo left a comment

Choose a reason for hiding this comment

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

Changes look good to me, but I haven't tested locally.

WDYT of replacing .travis.yml with a .github/workflows/ci.yml as @jfinkels did in posterior/goftests#23? Directly in this PR? Do we need to release goftests before that will pass?

Comment on lines 40 to +42
def test_model():
for name in MODULES:
yield _test_model, name
_test_model(name)
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to eventually switch from nose to pytest and make this a pytest fixture, but that's out of scope for this PR...

@fritzo
Copy link
Member

fritzo commented Feb 19, 2024

@emilyfertig I've released goftests 0.3.0 and parsable 0.3.0 which should now support python 3. Could you test again? LMK if you need any else.

@emilyfertig
Copy link
Author

Thanks for releasing those! All loom unit tests are passing now, but I ran the pubmed example and got the following. It looks like another place in parsable that needs updating for py3?

Traceback (most recent call last):
  File "/usr/local/google/home/emilyaf/posterior_loom/loom/examples/pubmed/main.py", line 158, in <module>
    parsable.dispatch()
  File "/usr/local/google/home/emilyaf/.virtualenvs/loom/lib/python3.11/site-packages/parsable.py", line 125, in dispatch
    inspect.formatargspec(*inspect.getargspec(fun)),
    ^^^^^^^^^^^^^^^^^^^^^
AttributeError: module 'inspect' has no attribute 'formatargspec'. Did you mean: 'formatargvalues'?

@fritzo
Copy link
Member

fritzo commented Feb 21, 2024

Thanks, looks like I need stronger tests for parsable. Let me fix that and cut another release.

@emilyfertig
Copy link
Author

(I should have put that last comment on the loom PR discussion, not this one.)

distributions tests are all passing now, after increasing SAMPLE_COUNT to get a few of the GOF tests to pass.

I looked at replacing .travis.yml with .github/workflows/ci.yml, but I didn't do it because wasn't clear to me how to translate this piece of travis.yaml:

env:
  - FLAVOR=_cc
  - FLAVOR=_cc DISTRIBUTIONS_USE_PROTOBUF=1
  - FLAVOR=_cc_examples
  - FLAVOR=_cy
install:
  - make install$FLAVOR

Also, setup.py is likely still broken (I made some local changes to it to get the pip package to build, and when I remove those changes it now appears to work, but I suspect I'm not building it clean.)

Besides travis.yaml and setup.py, I think this is now mergeable.

@fritzo
Copy link
Member

fritzo commented Feb 21, 2024

@emilyfertig I can take a look at the travis -> github/workflows migration, and look at setup.py as part of that. If I have trouble I may ask for help if you recall what you did.

@fritzo fritzo merged commit c2a9dcc into posterior:master Feb 21, 2024
@emilyfertig
Copy link
Author

Thanks, @fritzo ! My changes to setup.py are here: 8074e53 The commented-out portions are what initially seemed necessary to get it working, but now I'm not sure.

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.

3 participants