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

feat: update metadata on ec2 spot termination for batch #2271

Merged
merged 2 commits into from
Feb 25, 2025

Conversation

felippemr
Copy link
Contributor

👋 this is my first time here, excited to be contributing to the project

I implemented this based on #2214 and #2207. Let me know if it looks ok and I can add tests.

@felippemr
Copy link
Contributor Author

maybe @savingoyal or @romain-intel could take a look?

@savingoyal savingoyal requested a review from madhur-ob February 14, 2025 18:45
@madhur-ob madhur-ob marked this pull request as ready for review February 17, 2025 17:25
@madhur-ob
Copy link
Collaborator

Hi @felippemr

I will try to take a look today.

@madhur-ob
Copy link
Collaborator

Hi @felippemr

Thanks a lot for the contribution...
The PR looks good, just left a minor comment..

Also, two follow ups:

@felippemr felippemr force-pushed the felippemr-issue-2214 branch from cfaf015 to 8ba64ab Compare February 24, 2025 17:10
@felippemr
Copy link
Contributor Author

felippemr commented Feb 24, 2025

The stubs test fail for python 3.13. Can you take a look at them if possible?

Yes I will take a look. Are there instructions for running it locally?

@felippemr
Copy link
Contributor Author

felippemr commented Feb 24, 2025

Have you been able to test this out using a flow containing @Batch decorator?

I will be honest that I don't use metaflow, I just wanted to contribute to an OSS project. If you point me to the instructions for running it I will make it work.

@madhur-ob
Copy link
Collaborator

madhur-ob commented Feb 24, 2025

@felippemr The way to run stubs test can be inferred from here: https://github.com/Netflix/metaflow/blob/master/.github/workflows/test-stubs.yml

i.e.

  • first install local clone of metaflow with pip install .
  • install stubs with metaflow develop stubs install --force
  • run the tests with cd ./stubs && pytest --mypy-ini-file test/setup.cfg --mypy-only-local-stub && cd -
    • you may have to run this twice as witnessed in max_attempts: 2 in the CI file

@madhur-ob
Copy link
Collaborator

I will be honest that I don't use metaflow, I just wanted to contribute to an OSS project. If you point me to the instructions for running it I will make it work.

No worries, I will try to test on my end.

@felippemr
Copy link
Contributor Author

https://github.com/Netflix/metaflow/blob/master/.github/workflows/test-stubs.yml

I followed the file you referenced and was able to run the tests locally. Thanks. Looking at possible failures now.

@felippemr
Copy link
Contributor Author

felippemr commented Feb 24, 2025

_________________________________________________________________ project_decorator_validity[python_version=3.12] _________________________________________________________________
/.../metaflow/stubs/test/test_stubs.yml:14: 
E   pytest_mypy_plugins.utils.TypecheckAssertionError: Invalid output: 
E   Actual:
E       """                                         (diff)
E     .../.pyenv/versions/metaflow3131/lib/python3.13/site-packages/metaflow-stubs/__init__.pyi:1233: note: "project" defined here (diff)
E     .../.pyenv/versions/metaflow3131/lib/python3.13/site-packages/metaflow-stubs/plugins/aws/aws_utils.pyi:28: SyntaxWarning: invalid escape s
equence '\/' (diff)
E     main:3: error: Argument 1 to "project" has incompatible type "type[MyFlow]"; expected "str"  [arg-type] (diff)
E     main:4: error: Too many positional arguments for "project"  [misc] (diff)
E     main:7: error: Too many positional arguments for "project"  [misc] (diff)
E     main:11: error: Unexpected keyword argument "foo" for "project"  [call-arg] (diff)
E     main:16: error: Value of type variable "FlowSpecDerived" of function cannot be "NotAFlow"  [type-var] (diff)
E     main:19: error: Argument 1 has incompatible type "Callable[[], Any]"; expected "type[Any]"  [arg-type] (diff)
E   Expected:
E     main:3: error: .*incompatible type.*expected "str"\s+\[arg-type\] (diff)
E     main:4: error: .*Too many positional arguments for "project"\s+\[misc\] (diff)
E     main:7: error: .*Too many positional arguments for "project"\s+\[misc\] (diff)
E     main:11: error: .*Unexpected keyword argument "foo" for "project"\s+\[call-arg\] (diff)
E     main:16: error: .*cannot be "NotAFlow".*\[type-var\] (diff)
E     main:19: error: .*incompatible type.*\[arg-type\] (diff)
E   Alignment of first line difference:
E     E: main:3: error: .*incompatible type.*expected "str"\s+\[arg-type\]
E     A:   """
E        ^

On my local the tests are failing consistently with python 3.13.1, but I am unsure why CI is passing now. Also notice that in the trace I provided it specifies 3.12, but it is running tests for other versions as well. Probably that is why it is failing on my local. Here's a breakdown of the issues from the trace above:

  1. Argument Type Mismatch:

    • The error message indicates that the first argument to the project function is expected to be a str, but it is receiving a Type[MyFlow].
  2. Too Many Positional Arguments:

    • The project function is being called with more positional arguments than it expects. Unsure what is going on here to be honest.
  3. Unexpected Keyword Argument:

    • The test is passing a keyword argument foo to the project function, which is not expected by the function's signature. Why is this different only for python 3.13?
  4. Type Variable Mismatch:

    • The error indicates that a type variable FlowSpecDerived cannot be NotAFlow. This suggests a type constraint violation where the provided type does not satisfy the expected type variable constraints.
  5. Callable Type Mismatch:

    • The error message indicates that an argument of type Callable[[], Any] is being passed where a Type[Any] is expected. This suggests a mismatch between a callable and a type.
  6. Syntax Warning:

    • There is a SyntaxWarning related to an invalid escape sequence \/ in one of the stub files. This warning might not directly cause the test to fail but should be addressed to avoid potential issues.

@felippemr
Copy link
Contributor Author

felippemr commented Feb 24, 2025

Somehow tests started passing now, I think they might just be flaky 🤷‍♂️ .

metaflow on  felippemr-issue-2214 [?] via 🐍 v3.13.1 (metaflow3131) took 2s 
❯ cd ./stubs && pytest --mypy-ini-file test/setup.cfg --mypy-only-local-stub && cd -
=============================================================================== test session starts ===============================================================================
platform darwin -- Python 3.13.1, pytest-8.3.4, pluggy-1.5.0
rootdir: .../Projects/metaflow/stubs
plugins: mypy-plugins-3.2.0
collected 132 items                                                                                                                                                               

test/test_stubs.yml ....................................................................................................................................                    [100%]

============================================================================== 132 passed in 55.26s ===============================================================================
.../Projects/metaflow

@madhur-ob
Copy link
Collaborator

Somehow tests started passing now, I think they might just be flaky 🤷‍♂️ .

That's why we have max attempts as 2 in the CI

@madhur-ob
Copy link
Collaborator

Hi @felippemr

I have tested your changes, they are good to go.

@savingoyal savingoyal merged commit 731854e into Netflix:master Feb 25, 2025
29 checks passed
@felippemr felippemr deleted the felippemr-issue-2214 branch February 25, 2025 16:19
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