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

Join failed to get groupBy module on different file structure #868

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MickJermsurawong
Copy link

@MickJermsurawong MickJermsurawong commented Oct 18, 2024

Summary

I made a quick repro and curious on your input. I think when people define chronon features, it's expected they follow the structure, but it can live within some directories.. In that way, i think allowing flexible top module should be allowed. And we should be able to refer to chronon features object like other piece of python code. In this PR,

  • i'm adding a new unit test, simply importing the chronon features in unit test, and pytest api/py/test/test_sample.py will fail
api/py/test/sample/joins/quickstart/training_set.py:20: in <module>
    from group_bys.quickstart.purchases import v1 as purchases_v1
E   ModuleNotFoundError: No module named 'group_bys'
  • i have to name change namespace import from group_bys.quickstart.returns to sample.group_bys.quickstart.returns to make import work, now it fails couldn't find the module, although it's defined in a different file.
ERROR project/chronon_features/chronon_features/tests/joins/test_quickstart_training_set.py - 
ValueError: [GroupBy] Must specify a group_by name if group_by is not defined in separate file. You may pass it in via GroupBy.name.
  • Lastly, i updated the check and test pass

Why / Goal

Test Plan

  • Added Unit Tests
  • Covered by existing CI
  • Integration tested

Checklist

  • Documentation update

Reviewers

Signed-off-by: Mick Jermsurawong <[email protected]>
Signed-off-by: Mick Jermsurawong <[email protected]>
Signed-off-by: Mick Jermsurawong <[email protected]>
@nikhilsimha
Copy link
Contributor

ah, the fix here is to set python path to your top level chronon configuration directory.

if vscode recognizes the path, compile.py should too.

@MickJermsurawong
Copy link
Author

thanks nikhil, yup appending python path works.. for now i have to add add pythonpath in unit test, wrapping for compile.py, and add ignore on mypy. I think it would be great if we can treat these definitions like native python resources using the standard python module references

@nikhilsimha
Copy link
Contributor

They should work like standard python references actually - it is just that the python env needs to be setup to recognize chronon on the module. Or maybe I am misunderstanding, is it a wrong assumption for the framework to expect the top level chronon directory (sample in this case) to be on python path?

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.

2 participants