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

Correct path for Poetry cached libraries on Windows #6403

Merged
merged 53 commits into from Sep 17, 2020
Merged

Correct path for Poetry cached libraries on Windows #6403

merged 53 commits into from Sep 17, 2020

Conversation

alwx
Copy link
Contributor

@alwx alwx commented Aug 13, 2020

Proposed changes:

  • update CI to include the correct path to Poetry cached libraries on Windows

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@alwx alwx changed the title Printing poetry cache Correct path for Poetry cached libraries on Windows Aug 13, 2020
@alwx alwx requested a review from m-vdb August 13, 2020 15:28
@alwx alwx marked this pull request as ready for review August 13, 2020 15:28
@m-vdb
Copy link
Collaborator

m-vdb commented Aug 14, 2020

@alwx i've re-run the build to see if the cache is picked up (because I didn't see it in the last build)

@m-vdb
Copy link
Collaborator

m-vdb commented Aug 14, 2020

It doesn't look like it's loading the cache :/

m-vdb
m-vdb previously requested changes Aug 14, 2020
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.

doesn't seem to work in CI :/

@alwx alwx requested a review from wochinge September 16, 2020 08:27
@alwx
Copy link
Contributor Author

alwx commented Sep 16, 2020

It took a while to finish it, but now cache works for both Windows and Ubuntu!

@alwx alwx requested a review from m-vdb September 16, 2020 08:28
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this! My comments are mostly questions for my own understanding 😆

@@ -95,10 +95,10 @@ pika = "~1.1.0"
jsonschema = "~3.2"
packaging = ">=20.0,<21.0"
pytz = ">=2019.1,<2021.0"
rasa-sdk = "^2.0.0a4"
rasa-sdk = "^2.0.0a5"
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need to change that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated sanic in the latest version of rasa-sdk, and the new version of sanic has better Windows support. We had a discussion about it while you were on vacation :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw it but wouldn't it automatically pick the latest sdk version and latest sanic version (without changing the minimal alpha version)?


from rasa.core.training.converters.story_markdown_to_yaml_converter import (
StoryMarkdownToYamlConverter,
)
from rasa.shared.constants import LATEST_TRAINING_DATA_FORMAT_VERSION


@pytest.yield_fixture(scope="session")
def event_loop(request: Request) -> Iterator[asyncio.AbstractEventLoop]:
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this needed? If the scope is session it should go into conftest imo. If this just for the module, then the scope should be module

key: ${{ runner.os }}-poetry-${{ matrix.python-version }}-${{ hashFiles('**/poetry.lock') }}
restore-keys: ${{ runner.os }}-poetry-${{ matrix.python-version }}
path: .venv
key: ${{ runner.os }}-poetry-${{ matrix.python-version }}-${{ hashFiles('**/poetry.lock') }}-4
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the -4 for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's a mistake (just because I was testing cache, will update in a sec)

.github/workflows/continous-integration.yml Show resolved Hide resolved
.github/workflows/continous-integration.yml Show resolved Hide resolved
.github/workflows/continous-integration.yml Show resolved Hide resolved
.github/workflows/continous-integration.yml Show resolved Hide resolved
@alwx alwx requested a review from wochinge September 16, 2020 14:04
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

Can we re-run the workflow once? The last run didn't use the cache 😬

data/test_config/config_empty_en_after_dumping_windows.yml Outdated Show resolved Hide resolved
@@ -95,10 +95,10 @@ pika = "~1.1.0"
jsonschema = "~3.2"
packaging = ">=20.0,<21.0"
pytz = ">=2019.1,<2021.0"
rasa-sdk = "^2.0.0a4"
rasa-sdk = "^2.0.0a5"
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw it but wouldn't it automatically pick the latest sdk version and latest sanic version (without changing the minimal alpha version)?

.github/workflows/continous-integration.yml Show resolved Hide resolved
@alwx alwx merged commit fe0fed4 into master Sep 17, 2020
@alwx alwx deleted the poetry-cache branch September 17, 2020 14:36
# the project itself, which also makes it easier for us to determine the correct directory
# that needs to be cached.
run: |
poetry config virtualenvs.in-project true
Copy link
Collaborator

Choose a reason for hiding this comment

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

@alwx @wochinge wondering if we should do this for both ubuntu and windows, for the sake of keeping it simple 🤔

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.

None yet

3 participants