Fixup the test PyPI install workflow.

[cdmatters]

The current test_package.yml workflow test has been broken for 4 months. I assume it aims to test the nle packages from PyPI but it seems a bit confused, since there is a clone step in it. Furthermore I cant tell if the purpose of this workflow changed, since the description of the key installation step changed from 'Install from repo in test mode" to "Install nle via pip" a few months ago to accurately reflect the executed command. Perhaps this was a copy job from a workflow that tests installation from the repo?

At any rate the workflow was failing due to the fact that the cloned local nle was being accessed in the final step and it doesnt have the pynethack built, since nle was installed via pip. Removing the cloned repo has the desired effect.

Old test
New test (still fails due on 3.5 due to f strings)

Btw to trigger this new workflow I had to add a section to the yaml, which should be visible in the intermittent PR history.

[edran]

I assume it aims to the test the nle packages from PyPI but it is seems a bit confused, since there is a clone step in it.

I'm 90% sure I wrote it this way because we weren't including the test files / requirements into the package.

EDIT: which somehow I never got around to writing? 🙄

[edran]

I would actually put this back, and run the tests from the cloned repo?

[edran]

I would actually put this back, and run the tests from the cloned repo?

[cdmatters]

The only problem with doing this is that the versions will no longer match up. If you are installing from PyPI, you need to run the tests from that version, otherwise normal development will break this run regularly.

In general if we aren't going to package tests, I would argue it is better to focus on catching bugs before shipping, and then just make sure the package can be installed and core functionality run. Perhaps we could add a little more to the bash script run instead?

EDIT (I have added step and reset to the command, either way)
EDIT 2 I have also added a test run here

[cdmatters]

Also one more point - our README says we require Python 3.7+, but we have these tests that seem to suggest we support 3.5 and 3.6, even though some of these are officially at the end of their life.

I propose we either update the README or the tests we run. Generally I prefer to give as much support as possible, but in this case I would say it really comes down to what y'all had in mind when writing the library. It seems that everything tests fine on 3.6 up (based on our gh actions), and I think the only thing we might miss from 3.7 is that dicts are now in the language spec as ordered dicts by default (in 3.6 this is just an implementation detail for CPython). That said, I doubt we would rely on this anywhere. If we were to actively support 3.5 we'd also have to be careful not to use any f-strings.

LMK your thoughts and I can amend either the README or job entries here in this PR.

[heiner]

Busy weekend huh?

Personally I like the idea of supporting older versions of Python and have been avoiding f strings for that reason. I'm not sure we need to promise we'll do that forever (or on more of a "best effort" basis) but if it comes practically for free right now, why not?

[cdmatters]

Personally I like the idea of supporting older versions of Python and have been avoiding f strings for that reason. I'm not sure we need to promise we'll do that forever (or on more of a "best effort" basis) but if it comes practically for free right now, why not?

Yeah its kind of nice for all the people who might be running Ubuntu 16.04 but cant upgrade the built in 3.5 Python for whatever reason.

[cdmatters]

Alrighty @edran and @heiner got 2 changes requested, but i think I've addressed both of them. If you have any more LMK, otherwise I'll wait to merge until I get both approvals. 👍

[heiner]

Awesome, thanks!