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

Modify load_feature_tables() function to allow for absolute & relative paths #58

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

vishpillai123
Copy link

@vishpillai123 vishpillai123 commented Jan 24, 2025

I was running into issues with using this function and loading a TOML file from my local directory.

changes

  • Added is_absolute() to resolve whether path is relative or absolute
  • If referring to a path defined within module or an absolute path elsewhere, the function will retrieve the table.

context

  • I ran into an issue using this function for one of my schools (not PDP).

questions

None

I was running into issues with __file__ in a Jupyter notebook. I found that the issue was happening because typically __file__ is defined in a script or module setting, but not in a notebook. Also added a unit test.
@vishpillai123 vishpillai123 removed the request for review from bdewilde January 24, 2025 19:59
Copy link
Member

@bdewilde bdewilde left a comment

Choose a reason for hiding this comment

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

Can you show me an example of where the current code doesn't work?

)
except NameError:
# If __file__ is not available (i.e. Jupyter notebook), use the current working directory
pkg_root_dir = pathlib.Path(os.getcwd())
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pkg_root_dir = pathlib.Path(os.getcwd())
pkg_root_dir = pathlib.Path.cwd()

if p.parts[-1] == "student_success_tool"
)
except NameError:
# If __file__ is not available (i.e. Jupyter notebook), use the current working directory
Copy link
Member

Choose a reason for hiding this comment

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

I'm very surprised by this! I thought __file__ refers to the path to this module, no matter the env in which it's being imported. In fact I don't believe I've ever had this issue in a Notebook.

tests/modeling/test_utils.py Show resolved Hide resolved
Comment on lines 136 to 138
if expect_exception:
with pytest.raises(expect_exception):
utils.load_features_table(file_path)
Copy link
Member

Choose a reason for hiding this comment

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

In the case of "no error", standard practice is to use from contextlib import nullcontext as does_not_raise then with pytest.raises(does_not_raise) works in the same manner as an exception. You can find a couple examples of this pattern in other unit tests.

@vishpillai123
Copy link
Author

Can you show me an example of where the current code doesn't work?

Hi, so I'm trying to load a custom feature_table from my local directory (not for a PDP school), which is why I ran into this issue. But I also realized the rel_fpath is meant to refer to a path within the module.

Is there a way we can specify a path in our local directory? If not, how can we create that functionality?

I'm not sure if this PR is really solving this specific issue, so we can either close this out or if you have an idea of how to address, you can share and I can implement.

image

@bdewilde
Copy link
Member

@vishpillai123 There's probably a "well-behaved" way to handle either case: The user can pass a relative path to load files within the src code, or an absolute path to load from a file wherever it may live on disk. Does that make sense? Can you implement that logic? Potentially useful: https://docs.python.org/3/library/pathlib.html#pathlib.PurePath.is_absolute

also adjusted unit test and added "does_not_raise()"
@vishpillai123 vishpillai123 removed the request for review from kaylawilding January 27, 2025 16:23
@vishpillai123 vishpillai123 changed the title Modify load_feature_tables() function to be also used in a Notebook setting Modify load_feature_tables() function to allow for absolute & relative paths Jan 27, 2025
@vishpillai123
Copy link
Author

@bdewilde Made some updates based on your recommendation! Let me know what you think :). I also tested it for my school and it worked so this addresses my concerns. Previous functionality with relative path is the same.

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