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

Add Jupyter Dependencies + aiohttp to pyproject.toml #39

Merged
merged 4 commits into from
Feb 13, 2024

Conversation

nina-msft
Copy link
Contributor

@nina-msft nina-msft commented Feb 13, 2024

Description

Most users of PyRIT interact with Jupyter Notebooks when ramping up, or when leveraging library functionality. Users have given feedback that setting up with these notebooks is confusing. This is a step to move dependencies from dev --> all that relate to Notebook usage so users don't need to also install [dev] dependencies.

  • Adding ipykernel jupyter to pyproject.toml dependencies (general)
  • jupyter also added to dependencies (general) to address "TqdmWarning: IProgress not found. Please update jupyter and ipywidgets. See https://ipywidgets.readthedocs.io/en/stable/user_install.html" that was present when only ipykernel was moved.
  • cleanup ordering of deps, to be alphabetical

Tests

  • no new tests required
  • new tests added
  • existing tests adjusted

Documentation

  • no documentation changes needed
  • documentation added or edited
  • example notebook added or updated

@nina-msft nina-msft changed the title Users/nichikan/2062 Add Jupyter Dependencies + aiohttp to pyproject.toml Feb 13, 2024
Copy link

github-actions bot commented Feb 13, 2024

Test Results

93 tests  ±0   93 ✅ ±0   9s ⏱️ ±0s
 1 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit 30634ba. ± Comparison against base commit 58b8c7c.

♻️ This comment has been updated with latest results.

@rlundeen2
Copy link
Contributor

rlundeen2 commented Feb 13, 2024

(I think) this was used as a workaround addressed by this bug #40

So I think we may not need this?

@nina-msft
Copy link
Contributor Author

So I think we may not need this?

which part isn't needed @rlundeen2 ? The addition of aiohttp to dependencies?

Copy link
Contributor

@romanlutz romanlutz left a comment

Choose a reason for hiding this comment

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

Looks great! Feel free to remove or keep aiohttp depending on what @rlundeen2 does in his PR.

@nina-msft
Copy link
Contributor Author

Looks great! Feel free to remove or keep aiohttp depending on what @rlundeen2 does in his PR.

Thanks @romanlutz, I removed aiohttp and reordered the dependencies to be in alphabetical order (aka the larger diff rn)

@nina-msft nina-msft merged commit b37578b into main Feb 13, 2024
7 checks passed
@nina-msft nina-msft deleted the users/nichikan/2062 branch February 13, 2024 19:46
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