PrefectHQ / prefect-ray

Prefect integrations with Ray
https://prefecthq.github.io/prefect-ray/
Apache License 2.0
63 stars 5 forks source link

Update tests #7

Closed ahuang11 closed 2 years ago

ahuang11 commented 2 years ago

Succesfully tested locally.

ahuang11 commented 2 years ago

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

zanieb commented 2 years ago

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

desertaxle commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

"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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

ahuang11 commented 2 years ago

Do we have a public repository for Orion?

zanieb commented 2 years ago

@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 commented 2 years ago

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

zanieb commented 2 years ago

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

ahuang11 commented 2 years ago

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 commented 2 years ago

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

ahuang11 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

Okay this and https://github.com/PrefectHQ/prefect-dask/pull/2 is ready for review!

ahuang11 commented 2 years ago

@madkinsz Is this ready for merging?