Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Update tests #7

Merged
merged 27 commits into from
Jun 3, 2022
Merged

Update tests #7

merged 27 commits into from
Jun 3, 2022

Conversation

ahuang11
Copy link
Contributor

@ahuang11 ahuang11 commented May 10, 2022

Succesfully tested locally.

@ahuang11
Copy link
Contributor Author

ahuang11 commented May 11, 2022

from prefect.testing.fixtures import hosted_orion_api, use_hosted_orion needs new Orion released

Don't know what to do with 3.10 tests since I don't think ray supports it

@ahuang11 ahuang11 requested review from zanieb and desertaxle May 11, 2022 21:16
@zanieb
Copy link
Contributor

zanieb commented May 11, 2022

I'd exclude 3.10 tests/support. The release should be out tomorrow.

@desertaxle
Copy link
Member

desertaxle commented May 12, 2022

I think we also want to take the current documentation for the RayTaskRunner and include it in this collection's docs.

@tpdorsey do you have any guidance on how handle the RayTaskRunner docs in main docs once the RayTaskRunner has been moved out of the prefect package?

@tpdorsey
Copy link
Contributor

I think we also want to take the [current documentation for the RayTaskRunner]9https://orion-docs.prefect.io/concepts/task-runners/#running-tasks-on-ray) and include it in this collection's docs.

@tpdorsey do you have any guidance on how handle the RayTaskRunner docs in main docs once the RayTaskRunner has been moved out of the prefect package?

My inclination is to keep the task runner docs organized as is, but make any changes necessary to demonstrate using RayTaskRunner from a collection. This applies to both Task Runners concepts and tutorials. If there's some overlap between the collection docs and Prefect docs, that's fine and probably good.

Same for Dask presumably. This was my plan in anticipation of moving these task runners to collections.

@ahuang11
Copy link
Contributor Author

"If there's some overlap between the collection docs and Prefect docs, that's fine and probably good."

If one docs was updated, wouldn't this lead to outdated material? Would copying over the docs over and dropping a link work better?

@tpdorsey
Copy link
Contributor

Are we anticipating a lot of change? I think these runners are important enough that a small amount of work to keep docs in sync is warranted. I would rather have a better experience for users.

@ahuang11
Copy link
Contributor Author

ahuang11 commented May 12, 2022

Can you share which parts of docs should be migrated and how it should be done? Is it a separate md file? Or just a module header docstring?

@tpdorsey
Copy link
Contributor

@ahuang11 let's loop back on this. I am not yet familiar with how we're doing collections docs, nor with using collections. I'd like to familiarize with that before making a determination.

That said, I think we're going to have to adapt the documentation in both cases. It won't be a copy-paste. But I'll have a better answer for you asap.

@ahuang11
Copy link
Contributor Author

ahuang11 commented May 12, 2022

I'd exclude 3.10 tests/support. The release should be out tomorrow.

I think https://github.com/PrefectHQ/orion/tree/main/src/prefect/testing needs an __init__.py to import testing

I tried pip install "prefect>=2.0b4", but that doesn't have testing

site-packages: cd prefect
(base) [Thu 12 17:50] prefect: ls
__init__.py	_version.py	blocks		client.py	deployments.py	exceptions.py	flows.py	logging		profiles.toml	settings.py	task_runners.py	utilities
__pycache__	agent.py	cli		context.py	engine.py	flow_runners.py	futures.py	orion		serializers.py	states.py	tasks.py
(base) [Thu 12 17:50] prefect: cd testing
cd: no such file or directory: testing

@ahuang11
Copy link
Contributor Author

ahuang11 commented May 13, 2022

I think may require another release of Orion for the tests to pass (with __init__.py in the testing directory):

  1. 2.0b4 doesn't include testing dir
  2. cant pull from git main because it's not public

@zanieb
Copy link
Contributor

zanieb commented May 13, 2022

re 2. you can install from the commit in the public repository once we've got a fix

@ahuang11
Copy link
Contributor Author

Do we have a public repository for Orion?

@zanieb
Copy link
Contributor

zanieb commented May 13, 2022

@ahuang11 I updated it to the public install and you changed it back :P We want to install from the orion branch on the public prefect repository.

Edit-1: I see it failed in CI. Weird. It installs locally with pip install -r
Edit-2: This is an issue with setuptools vs requirements formatting. I force pushed another commit.

@ahuang11
Copy link
Contributor Author

@madkinsz do you know if this 3.7 tests hanging is the same as https://github.com/PrefectHQ/orion/pull/1696

@zanieb
Copy link
Contributor

zanieb commented May 13, 2022

That deadlock shouldn't be a thing anymore, so I'm not sure.

@ahuang11
Copy link
Contributor Author

Okay I think I fixed it; had to do with the event loop which I copied over from orion
https://github.com/PrefectHQ/orion/blob/main/tests/conftest.py#L153-L190

@madkinsz Should that fixture be put under prefect.testing so it can be imported rather than copied?

@zanieb
Copy link
Contributor

zanieb commented May 19, 2022

Hm that's a special fixture I'm not sure we can move it. Let's just stick with this for now.

@ahuang11
Copy link
Contributor Author

@madkinsz to fix the py37 tests, I simply removed the 3.7 pin from
https://github.com/PrefectHQ/orion/pull/1598/files#diff-4d7c51b1efe9043e44439a949dfd92e5827321b34082903477fd04876edb7552R28

But I was wondering, was there a special reason to pin an older version?

@zanieb
Copy link
Contributor

zanieb commented May 19, 2022

@ahuang11 I do not remember the reason that was necessary. However, if you're only going to support that newer version of Ray you should update the code for connecting to a cluster.

@ahuang11
Copy link
Contributor Author

Okay this and PrefectHQ/prefect-dask#2 is ready for review!

@ahuang11 ahuang11 requested a review from desertaxle May 19, 2022 19:45
@ahuang11
Copy link
Contributor Author

@madkinsz Is this ready for merging?

@desertaxle desertaxle merged commit 7acb13f into main Jun 3, 2022
@desertaxle desertaxle deleted the update_tests branch June 3, 2022 21:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants